Community
Participate
Working Groups
As we are allowing more and more background jobs to do work in RSE, some of these jobs want to notify the System about state changes and send e.g. a Refresh event. Such events are typically expected to be delivered on the dispatch thread. But currently, SystemRegistry.fireEvent() just uses the current thread of the caller, which leads to SWT.InvalidThreadAccess exceptions which we have recently seen in the org.eclipsecon.tmtutorial for instance. I think there are three possible solutions to this: A) Write up API Docs for fireEvent() that tells clients that they always need to switch to the display thread themselves before calling it. IMHO, this leads to duplication and is error prone. B) Add a check to fireEvent() which tests if it is running on the display thread already; if yes, send event synchronously and if no, post it to the event queue asynchronously. IMHO, this may be problematic because the semantics of it is not always clear, and the extra thread check is unnecessary work for clients that do know which thread they are on. C) Add methods postEvent() which always posts using asyncExec() and sendEvent() which requires callers to be on the display thread. Deprecate fireEvent() but add delegating code which checks for the current thread and delegates either to sendEvent() or postEvent(). In my opinion, (C) is best. In order to reduce the number of methods in SystemRegistry, we should do the new sendEvent() / postEvent() methods only once and avoid convenience variants of them. Comments ?
Here is one example of a backtrace from the Eclipsecon tutorial, which occurs because SubSystem$ConnectJob doesnt switch to the display thread correctly for firing the event. org.eclipse.swt.SWTException: Invalid thread access at org.eclipse.swt.SWT.error(SWT.java:3478) at org.eclipse.swt.SWT.error(SWT.java:3401) at org.eclipse.swt.SWT.error(SWT.java:3372) at org.eclipse.swt.widgets.Widget.error(Widget.java:432) at org.eclipse.swt.widgets.Widget.checkWidget(Widget.java:326) at org.eclipse.swt.widgets.Widget.getData(Widget.java:485) at org.eclipse.rse.internal.ui.view.monitor.MonitorViewWorkbook.getCurrentTabItem(MonitorViewWorkbook.java:156) at org.eclipse.rse.internal.ui.view.monitor.MonitorViewWorkbook.getViewer(MonitorViewWorkbook.java:180) at org.eclipse.rse.internal.ui.view.monitor.SystemMonitorViewPart.getViewer(SystemMonitorViewPart.java:639) at org.eclipse.rse.internal.ui.view.monitor.SystemMonitorViewPart.systemResourceChanged(SystemMonitorViewPart.java:805) at org.eclipse.rse.internal.model.SystemResourceChangeManager.notify(SystemResourceChangeManager.java:74) at org.eclipse.rse.model.SystemRegistry.fireEvent(SystemRegistry.java:2955) at org.eclipse.rse.model.SystemRegistry.connectedStatusChange(SystemRegistry.java:2860) at org.eclipse.rse.model.SystemRegistry.connectedStatusChange(SystemRegistry.java:2844) at org.eclipse.rse.core.subsystems.SubSystem$ConnectJob.performOperation(SubSystem.java:1590) at org.eclipse.rse.core.subsystems.SubSystem$SubSystemOperationJob.run(SubSystem.java:1274) at org.eclipse.core.internal.jobs.Worker.run(Worker.java:58)
Browsing through Eclipse code a little, it seems to me that variants of fireEvent() are only used when (SWT) widgets or other UI elements communicate with each other, so it is obvious that all kind of event firing is always on the display thread. More generic kinds of event handling, however, seem to always use variants of postEvent() / sendEvent() where sendEvent is often "more hidden" than postEvent and used for internal kinds of communication only.
Based on the fact that the variants usually fire events from UI code, what do you think we should do? Do you think think we should explicitly switch to the Display thread or do you think introduce other ways to fire events (i.e. postEvent() and sendEvent())?
Not for 2.0
I'd like to take a first stab at this in 2.0 because it's causing some exceptions in the current code. I'll make my changes so that no APIs actually change.
I've added code to put the event firing on the main thread whenever it is not already on it.
So, Dave - the bug status should be FIXED then, shouldn't it? We should consider this VERY carefully, because fireEvent() is a method in ISystemRegistry for which we have planned implementation to go into non-UI. In a headless RSE in the future, we will not have UI so there is no concept of a display thread and no access to SWT.Display. At any rate, we need to update Javadoc to clearly specify what we do. I'd recommend specifying that if we are running with UI, fireEvent() will switch to the display thread; but in a headless environment, it can be any other thread. This means that UI components which register themselves as listeners can be sure they are notified on the display thread. Non-UI components should be aware that it can be any other thread. In order to implement this, in the future, SystemRegistryUI would need to set the EventNotifier in SystemRegistry so it can do the event switching. We should perhaps discuss this quickly at the committer meeting since it is a very important change.
marking fixed to ease release notes generation and tracking
*** Bug 174775 has been marked as a duplicate of this bug. ***
[target cleanup] 2.0 RC1 was the original target milestone for this bug
