Community
Participate
Working Groups
If the SWT had an unsynchronized Display.isCurrentThread() method then it would allow for trapping when UI operations trigger I/O operations over potentially slow network or other IO which would otherwise lead to a UI hang. Currently this cannot be implemented with Display.getThread() because that it syhnchronized, which means that if you call it and something is already running on the Display class that it won't be able to make progress to return 'false'. The implementation would look like: public boolean /*not synchronized*/ isDisplayThread() { return Thread.currentThread() == thread; } I'd be happy to contribute this if someone could point out how to make/test changes to SWT.
For example, if a UI hang is seen in JGit, a breakpoint in Eclipse can be added to the FileRepository.openPack() method to trigger iff Display.getCurrent().isDisplayThread(). When a reproducible test case occurs, the debugger will automatically open to that thread, which will include the back traces of how that UI violation occurred. This will also help catch the platform delays, which are otherwise only seen if the delay is more than a certain amount. Note that it is possible to do this at the moment by using reflection, but this adds overhead to the debugging mechanism.
I'm an intern and as such I'm not on expert on Display and concurrency, but isn't the general notion to make long-lasting execution run on a background thread and not on a UI thread, and then use a progress bar for things that last too long? I.e, instead of improving how the UI handles heavy threads, wouldn't it make more sense making sure that there are no long-running threads on the UI and run such things in background threads? But I may be missing something here.
Then there is also display.async() (as oppose to display.sync()) for running things asynchronously?
Yes, that's exactly what people should be doing. However, there are many places in the existing Eclipse codebase where due to various levels of indirection it's possible to have a UI job blocked on the result of a synchronous method call that's depending on the result of an IO operation. For example, various Label Decorators (which must get updated in the UI thread) may end up going down into the file to determine its status, which if it is on a network share may cause delays. The problem is this can't be easily debugged when it happens in Eclipse at the moment. The way to identify it - other than the UI pauses, which are highlighting this on a regular basis - is to put a conditional breakpoint on a known IO operation (File.read, socket etc.) and use a condition to say 'Is this the UI thread'. In the case where it is in the UI thread, any delays in the IO will cause unresponsiveness in the UI. The way to test this is Thread.currentThread() == Display.thread However, the thread field is private, so Thread.currentThread() == Display.getThread() would be done. This isn't right either, because Display.getThread() is synchronized, which means that either (a) you'll cause deadlock on the Display locking against itself, or (b) you have a background thread which wasn't on the display thread (i.e. this will return 'false') but now you've suddenly made it depend on the Display lock, which means it is now possible to block the UI thread (albeit briefly). Since removing synchronized is probably the wrong thing - and in fact, in most cases, the calls are all of the form Display.getThread() == Thread.currentThread() in the codebase - by adding this method you not only simplify it but also allow other user in the codebase to determine whether it is on the UI or not. For example, this is common: if(Display.getThread() == Thread.currentThread() { Display.sync(new Runnable() { ... } ) } else { Display.async(new Runnable() { ... } ) } but in this case, the Display.getThread() will lock the display object. Finally, the method is more meaningful ("Am I the display thread?") rather than imperative ("Give me the display thread so I can compare"). Should SWT ever move to multi-threaded display processing - say, one thread per display - then "Am I the display thread?" can be reimplemented to check the appropriate display if necessary, whereas existing code would break. So: this is not about how UI jobs should be scheduled, it's about making code cleaner and allowing accidental bad uses to be captured at debug time.
Thank you for the detailed explanation. It took a while to understand the business above, but your logic is sound and clear. I've read all of this business: ~When it's safe to use Volatile variables~ http://www.ibm.com/developerworks/java/library/j-jtp06197/index.html From what I gather by reading the source code and the article above, your snippet of code above would follow the "One-time publication" pattern: http://www.ibm.com/developerworks/java/library/j-jtp06197/index.html#listing3 Because the "thread" variable is effectively immutable (that is is only mutated once when it is created. (and when disposed..).). As such, from a pure technical point of view, it looks safe to me to add such a method. I also see the benefit that having such a method would improve U.I responsiveness. I could add the following to the Gtk/Cocoa/Win32/(wpf?) Display class and submit it for review. public boolean /*not synchronized*/ isDisplayThread() { return Thread.currentThread() == thread; } Just two things: 1) Any thoughts on how to test this business? Is there a snippet of code that we could run to evaluate effectiveness? 2) @Alex/Arun/ Gtk / Cocoa / Win32 folks, what are your thoughts on the matter?
Yes, that's the change/implementation required. To test this, you can run: final boolean success[] = { false }; Display.sync(new Runnable() { success[0] = Display.isCurrentThread() } ) assertTrue(success[0]); success[0] = false; Thread t = new Thread() { run() { success[0] = !Display.isCurrentThread(); } }; t.start(); t.wait() assertTrue(success[0])
Great proposal. @SWT team: can we consider this for M7? Last chance for this release, and the change looks small and clean.
Leo, are you working on a patch or do you want me to provide a first cut?
The new API looks simple & straightforward and seems like something that would be useful in debugging UI hangs. But, the API request came in too late to be evaluated for M6. Since we are already past API freeze (M6), any API changes would require PMC approval.
Ok so can we get PMC approval? Since this is an addition and not a change it should be minimal, and can be used to improve the UI hangs which is a key part of the Mars release.
Before adding this, I'd like to see an actual use case where it actually works. In all the cited examples (JGit, FileInputStream, Socket), a conditional breakpoint expression like org.eclipse.swt.widgets.Display.getDefault().getThread() == Thread.currentThread() doesn't even compile because the surrounding class doesn't depend on SWT. On the other hand, you can already use this breakpoint condition in java.io.FileInputStream#read() and get interesting results: "main".equals(Thread.currentThread().getName())
New Gerrit change created: https://git.eclipse.org/r/44212
The easiest way to debug in that case is to put an Import-Package: org.eclipse.swt in the Manifest.MF of the JGit project and/or add the SWT jar to the .classpath, so that the breakpoint can recognise it. If you have a UI project (e.g. EGit) then the breakpoint will be triggered without error. Note that this is also useful for other (non-debugging) applications - for example, an operation that needs to be run on the UI thread by detecting whether it's already on the UI thread or not and shelling out - or alternatively, when implementing an action that needs to run off the UI thread and needs to explicitly check whether it's on the UI thread or not. Finally this could be implemented as a generic test in an E4 application layer that ultimately called down to Display.isCurrentThread() or not. There are already plenty of cases in Eclipse where this happens. --- if (isVisible() && widget != null) { Display display = widget.getDisplay(); if (display.getThread() == Thread.currentThread()) { update(e.getProperty()); } else { display.asyncExec(new Runnable() { @Override --- boolean isValidThread () { return control.getDisplay ().getThread () == Thread.currentThread (); } --- if (display != null) { if (display.getThread () == Thread.currentThread ()) { display.setData (NO_INPUT_METHOD, "true"); //$NON-NLS-1$ } --- boolean isUI() { return (!display.isDisposed()) && (display.getThread() == Thread.currentThread()); } --- // Only bother with the job if in the UI Thread if (Thread.currentThread() == PlatformUI.getWorkbench().getDisplay() .getThread()) { fireListeners(event); return; }
@Alex Blewitt: Here is the relevant commit and jUnit test cases. Is this what you had in mind? https://git.eclipse.org/r/#/c/44212/ @Testing I included two extra jUnit test cases into SWT Tests. I tested on Gtk2/Gtk3. But we ought to test on Cocoa/Win32/Aix (Aix has different Thread structure). I don't think this will break anything, because code like this is already indirectly called asyncronosly from public api beep & Tracker. E.g: Display:beep() -> isValidThread -> (return thread == Thread.currentThread ();) But it wouldn't hurt to double check on these platforms. @M7 milestone: I think this is best answered by folks with SWT contributor rights. Alex.K/Arun/Markus etc.. thoughts?
Display.isCurrentThread() ->> Display.isDisplayThread() as per gerrit comment.
Updated patch as per comments.
I added style changes in the patch. The gerrit build fails: [ERROR] The method isDisplayThread() is undefined for the type Display As Alex.K mentioned: "when building with -P build-individual-bundles target platform for every bundle is calculated from the p2 repos. Aka tests bundle is compiling against the old swt bundle, this resolving doesn't happen in the full build." It seems that tests don't see the new api yet as they are using a cached swt and not the newly included swt. Perhaps we should continue testing on Win32/Cocoa. Is there anyone who could download the patch and run the two jUnit tests on these platforms?
Build failure probably due to: https://bugs.eclipse.org/bugs/show_bug.cgi?id=458228
The test new cases don't fail if Display#isCurrentThread() is made synchronized. The test methods should have a name that matches the tested method. (In reply to Alex Blewitt from comment #0) > Currently this cannot be implemented with Display.getThread() because that > it syhnchronized, which means that if you call it and something is already > running on the Display class that it won't be able to make progress to > return 'false'. You still didn't give a single case where the SWT calls back to client code while keeping a lock on the Device. Without a pressing need, this is not M7-worthy. > public boolean /*not synchronized*/ isDisplayThread() { > return Thread.currentThread() == thread; > } The /*not synchronized*/ should be after "public". The Javadoc has formatting error (*).
(In reply to Markus Keller from comment #19) > The test new cases don't fail if Display#isCurrentThread() is made > synchronized. The test methods should have a name that matches the tested > method. > > (In reply to Alex Blewitt from comment #0) > > Currently this cannot be implemented with Display.getThread() because that > > it syhnchronized, which means that if you call it and something is already > > running on the Display class that it won't be able to make progress to > > return 'false'. > > You still didn't give a single case where the SWT calls back to client code > while keeping a lock on the Device. Without a pressing need, this is not > M7-worthy. > > > public boolean /*not synchronized*/ isDisplayThread() { > > return Thread.currentThread() == thread; > > } > > The /*not synchronized*/ should be after "public". > The Javadoc has formatting error (*). Thank you for feedback. Let's wait till after release to continue on this.
(In reply to Leo Ufimtsev from comment #20) > > Let's wait till after release to continue on this. Deferring to 4.6 accordingly.
Looks like Leo is away(?). Moving to M3.
No plans for me to work on this at the moment, re-assigning to inbox.
Moving out of M3 as nobody is working on it currently.
Moving out of 4.6 as we are past API freeze and currently no one is planning to work on this.
I'm back. Will try to look at this sometime.
New Gerrit change created: https://git.eclipse.org/r/84695
I updated the patch as per comments. Could someone review please? Works on Gtk. Should theoretically work on Cocoa/Win32. Could someone checkout the patch on Win32/Cocoa and run the jUnit test suite to confirm nothing breaks / patch works. Thanks.
@Alex Blewitt. There doesn't seem to be anyone interested in reviewing the Win32/Cocoa parts of this code. Do you have a specific use case for this in JDT/Platform.ui? I could take the time to setup a cocoa/win32 setup to test this, but my concern is that this might becomes an api that wouldn't actually be used. If you could submit a patch or actual use case for this api, show that it would actually benifit eclipse, then on my part I can spend some time setting up win32/cocoa test setup. Otherwise it might be that we have to abandon this patch based on lack of interest in parties involved.
(In reply to Leo Ufimtsev from comment #29) > @Alex Blewitt. > > There doesn't seem to be anyone interested in reviewing the Win32/Cocoa > parts of this code. > > Do you have a specific use case for this in JDT/Platform.ui? > > I could take the time to setup a cocoa/win32 setup to test this, but my > concern is that this might becomes an api that wouldn't actually be used. > > If you could submit a patch or actual use case for this api, show that it > would actually benifit eclipse, then on my part I can spend some time > setting up win32/cocoa test setup. > > Otherwise it might be that we have to abandon this patch based on lack of > interest in parties involved. Closing bug for now due to lack of respone from reporter. However, if above is met (i.e patch in platform.ui that utilizes this api) is provided, then I would be happy to continue work on this. However if not I want to avoid adding dead api to swt.
