Bug 574556

Summary:	proper concurrency control instead of dubious sleeping
Product:	[Eclipse Project] Platform	Reporter:	Jörg Kubitz <jkubitz-eclipse>
Component:	UI	Assignee:	Platform-UI-Inbox <Platform-UI-Inbox>
Status:	NEW ---	QA Contact:
Severity:	enhancement
Priority:	P3	CC:	wim.jongman
Version:	4.21
Target Milestone:	---
Hardware:	All
OS:	All
See Also:	https://bugs.eclipse.org/bugs/show_bug.cgi?id=574562 https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/182619
Whiteboard:

Description Jörg Kubitz

2021-06-30 03:14:31 EDT

I want to avoid avoid unnecessary sleeps during eclipse startup.
Setting a breakpoint to Thread.sleep() and starting eclipse lead me to this:

With bug 429363 a sleep was added in WorkbenchAdvisor to avoid a deadlock. Not only does sleep waste time - it is also not a guarantee to reliable solve the concurrency problem (example see bug 266985#c8).

The same problem was also already analysed in bug 188320.
The solution was to not use display.wake() in linux/GTK. see org.eclipse.swt.widgets.Synchronizer.asyncExec(null)

The way for a eventloop is to spin with
	while (fContinueEventDispatching) {
		if (!fDisplay.readAndDispatch()) {
			fDisplay.sleep();
		}
	}

and stop spinning with 
	fContinueEventDispatching= false;
	fDisplay.asyncExec(null); // <- bug 188320
See org.eclipse.jface.operation.ModalContext, org.eclipse.jface.operation.ModalContext
as example usecases.

I suggest to use the fixed asyncExec(null) instead of wait+wakeup.

However i do not like that wakeup() does not do what it suggests. I would rather like to see the fix of asyncExec() in wakeup() or deprecate wakeup() (which is used in another dozen places in eclipse with the same possible flaws).

Comment 1 Wim Jongman

2021-06-30 05:21:15 EDT

(In reply to Jörg Kubitz from comment #0)

Thanks for the analysis.

IIUC: wakeup() does not work and it's replacement is asynExec(null)?

Comment 2 Jörg Kubitz

2021-06-30 05:24:57 EDT

(In reply to Wim Jongman from comment #1)
> IIUC: wakeup() does not work and it's replacement is asynExec(null)?

Thats what the commits of bug 188320 suggest. I did not validate it.

Comment 3 Wim Jongman

2021-06-30 06:37:47 EDT

(In reply to Jörg Kubitz from comment #2)
> (In reply to Wim Jongman from comment #1)
> > IIUC: wakeup() does not work and it's replacement is asynExec(null)?
> 
> Thats what the commits of bug 188320 suggest. I did not validate it.

Ok, got it. So your suggested plan is to replace all Thread.sleep with fDisplay.sleep. 

Then wake up by using a fixed wakeup() OR, if that can't be done, use asyncExec(null) to wake up.

Comment 4 Jörg Kubitz

2021-06-30 07:15:17 EDT

(In reply to Wim Jongman from comment #3)

> Ok, got it. So your suggested plan is to replace all Thread.sleep with
> fDisplay.sleep. 

NO!! That two sleeps are totally different things. I suggest to remove the single Thread.sleep (see commit 182619) and use fDisplay.asyncExec(null) instead of wakeup.

Optional fix wakeup (bug 574562) too.