Community
Participate
Working Groups
During code review for bug 227750, I found that SystemPostableEventNotifier is currently implemented to run a Display.syncExec() which is totally against API Javadoc, which claims that this class posts events asynchronously. The code has been a syncExec ever since the initial population in the openRSE repository, even before our refactorings. Only bug 150919 has been applied to that class so far. The code is currently only used from SystemResourceChangeManager#postNotify() which is only called from SystemRegistryUI.postEvent(ISystemResourceChangeEvent) which is only called from DaytimeSubSystem#initializeSubSystem() where it's already wrapped in a Display.asyncExec() call. I think we should do the following: (1) Change DaytimeSubSystem to avoid the call since it's unnecessary. (2a) Consider removing SystemRegistryUI.postEvent() as well as SystemPostableEventNotifier -- what do committers think? (2b) As an alternative, fix the implementation to dow what's described (2c) Or, fix the API Javadocs to describe what's currently done In case we are not removing the API calls, I think the description and implementation should be changed to make use of the new IRSEInteractionProvider#asyncExec() logic in order to avoid unnecessary UI dependency. -----------Enter bugs above this line----------- TM 3.0RC2 testing installation : eclipse-SDK-3.4RC3 (I20080530-1730), cdt-5.0RC2 (200805231539), DSF-1.0rc2 (20080527), emf-2.4.0rc2, Findbugs-1.3.3, Releng.Tools-3.4RC3, RSE-3.0RC2, RXTX-2.1-7r3b, Subversive-0.7.0.v20080517, FTP_WebDav-3.2.2 org.tigris.MemMonitor, WR-Retriever-3.0.v20070604, RSE install : RSE 3.0RC2 in workspace, TM-terminal, TM-discovery java.runtime : Sun 1.6.0_05-b13 -Xmx512m -XX:MaxPermSize=128m os.name: : Windows XP 5.1, Service Pack 2 ------------------------------------------------ systemtype : Windows-local, Dstore-win, Dstore-linux targetos : Red Hat Enterprise Linux WS release 4 (Nahant Update 3) targetuname : Linux parser 2.6.9-34.EL #1 i686 athlon i386 GNU/Linux targetvm : Sun Java HotSpot(TM) Client VM (build 1.4.2_12-b03, mixed mode) ------------------------------------------------
I vote for (1) and (2a) unless we can find other product uses of this that would preclude its removal. If that were to happen I'd vote for (2c).
Does not seems we are affected (so far). I agreed with Dave D. Thanks.
Here is some more data: * Javadoc in ISystemRegistryUI#postEvent() explicitly says that Display.asyncExec() is used to post events into the Display thread. This doc has been in there since openRSE - initial (SystemRegistry v1.1). * The implementation, however, looks like it always used SystemPostableEventNotifier which did the syncExec() -- incorrectly, in my opinion. * There are additional callers of this through SystemRegistryUI#postEvent(ISystemResourceChangeListener, ISRCEvent) which I did not find in my first search. The callers are 3, all of which are in rse.internal.ui.view so they can be changed to use Display.asyncExec() themselves if needed: SystemViewAPIProviderForFilters SystemViewFilterAdapter SystemViewFilterReferenceAdapter * In SystemRegistry itself, we have the various fireEvent() variants. These used to be totally synchronous until 24-May-2007 (approx RSE 2.0RC1), when behavior was changed with bug 176601 to always fire events on the main Thread. Since then, the various fireEvent() variants work like this: * If already on main thread, fire event immediately and synchronously * If not on main thread, perform Display.asyncExec() I find this behavior slightly problematic because it's not clear for the caller whether the event will eventually be executed synchronously or asynchronously. I will do more investigations of this, because it's important to know our whole event posting story such that we can write migration docs and guidance in case we remove the postEvent() variants. My general feeling is, that migration notes should instruct users to run RSECorePlugin.getInteractionProvider().asyncExec() instead of the SystemPostableEventNotifier, in case we get rid of that class.
Likely won't be able to change behavior in 3.1 but look at the documentation again when the IRSEInteractionProvider is actually implemented as per bug 190231.
