Summary: | Listeners handling in TextViewer.java not implemented in a way suitable for concurrent processing | ||
---|---|---|---|
Product: | [Eclipse Project] Platform | Reporter: | Carsten Hammer <carsten.hammer> |
Component: | Text | Assignee: | Platform-Text-Inbox <platform-text-inbox> |
Status: | NEW --- | QA Contact: | |
Severity: | normal | ||
Priority: | P3 | CC: | daniel_megert, loskutov, twolf |
Version: | 4.14 | ||
Target Milestone: | --- | ||
Hardware: | All | ||
OS: | All | ||
Whiteboard: |
Description
Carsten Hammer
2019-12-22 03:45:03 EST
fyi: due to https://git.eclipse.org/r/#/c/154908/ being merged https://git.eclipse.org/r/#/c/154893/ now is not enough to solve the issue and you need additionally before that one a revert of https://git.eclipse.org/r/#/c/154908/ (and of course additional changes in whatever module make use of implementation details of TextViewer.java). Is anybody able to implement the suggested additional junit tests to cover the error case? Could recording a session using swtbot help? Carsten, the right approach would be not to use MT data structures or whatever else like synchronized etc (because we are in the UI thread), but to disallow modifications on the listener list during event processing (by using copies of the list, or, if possible, to use ListenerList instead of ArrayList). We do not have a problem of MT processing, we have a problem of re-entrant code during event processing in UI thread. Hey Andrey, thanks for your feedback. However I thought we agreed that https://git.eclipse.org/r/#/c/154565/ with the same problem only was helpful because the implementation was simply bad. If the implementation is clean and does not try to change the original list outside code where the iterator on the list is available then you do not get the problem. It is completely unclear why the code there and in all other places where this pattern is used is doing it that way. You can change the original list without creating an independent copy using the iterator on it. The only to some extend valid reason for creating copies of the original list is in MT environments and there the use of the right datastructures would solve your problem at the same time and much more elegant. Additionally using a Set would would move the checking for existing entries in the listeners object into the listeners implementation and this helps to increase readability. Maybe https://docs.oracle.com/javase/7/docs/api/javax/swing/event/EventListenerList.html does similar things - I am not familiar with this class that seems to be part of java swing or do you think of a different one? Creating a copy of the list as you suggest would not dissallow anything. It would allow two parts of the code run on different versions of the list - even when they run always in the same thread. One might think he is changing a list and in fact is changing a copy. Given that you are right and it under no circumstances is and never will be a MT access to these datastructures possible it still is the question why one should allow such "magic" constructs to be necessary to do such simple things. But don't get me wrong: if introducing copies of arraylist whenever eventprocessing is allowed to work on the original list should be the agreed approach currently it is fine for me. I just think it is not the best way currently. Maybe I am missing something to be able to understand the beauty of this approach. As far as I see the copies are done because a listener may unregister itself during the iteration. |