Community
Participate
Working Groups
Calling IIdentifier.removeIdentifierListener(IIdentifierListener) in an identifierChanged() handler will corrupt the event list resulting in only every 2nd listener receiving notification of the event. This is because the Identifier.fireIdentifierChanged method is iterating over the identifiers list with an index based for loop, while the removeIdentifierListener is concurrently modifying the same list. This defect exists in 3.8 as well. It is not obvious to us because we only ever register one listener with the identifier, the LayoutHelper. I discovered this while implementing some capabilities work in 4.2. I was registering a listener for each view and not all views where being notified when the Identifier changed.
I looked through some of the Eclipse code and it seems that org.eclipse.ui.actions and jface all use org.eclipse.core.runtime.ListenerList for managing lists of listeners. Among other run time characteristic claims of this class, the list is allows addition and removal of listeners while the a client is iterating over the results of ListenerList.getListeners() Perhaps we should be using this class too? If so, do we need to see where else we should be using this class instead of an ArrayList?
To cross-reference with some (old) history ... the ListenerList was made API via bug 94156. Some interesting comments there, such as, from John A. "It performs copy on write, which means that it will be possible for listeners to receive notification after they are removed." Not sure if that's relevant to this case ... but, I found it interesting enough to mention here. I know it is complicated, and there's lots "on the web" about listener's and their lists, such as http://www.thecoderscorner.com/tcc/a/java/java-threading/introduction-to-reducing-thread-contention-in-jav/page3 but I think the "use cases" boil down to whether or not listeners are added and removed a lot (compared to frequency of firing events events). That is, I'm not sure there's "one size that fits all" ... but, ListenerList would be better than managing your own ArrayList. HTH
Created attachment 204588 [details] Use ListenerList
Created attachment 204592 [details] Remove warnings from Identifier.java All warnings removed, but had to resort to one @suppress tag. Added a todo on that one but it would require reimplementing the safeCopy methods in Util to use Generics which is probably not something we should be spending our time on now.
How is the current implementation of bug 355497 affected by this defect (i.e. what would I look for to see the issue) ?
The current patch on bug 355497 does not require this fix. One of the outstanding capabilities defects described in comment 11 of that bug would, however, require it.
Cool, once the new patch is there we should review both at the same time...
(In reply to comment #3) > Created attachment 204588 [details] > Use ListenerList I was initially hesitant about releasing this but since the behaviour in which the listeners are notified do not appear to be specified in the javadocs, I don't feel that this is a contract that we have to keep. Fix released to R3_development and R4_development. Thanks for the patch, Dean! http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?h=R3_development&id=8f4c911caeca2d9c8109b2e3111a21de52bb13f2 http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?h=R4_development&id=4817a6c60e809af09bfa750b509648c6433f95d2
Verified on I20111014-1625
