Community
Participate
Working Groups
In the code of TextViewer.java there have been two serious bugs: 1. Use of a loop counter instead of an iterator. This loop counter in general cannot reflect changes when deleting a listener if used this way. In case of addition of a listener it works but in case of deleting a listener if leaves out listeners to process because the current position in the loop processing is not updated. 2. Use of a datastructure ArrayList that is in general not suitable for concurrent processing. Its iterator throw concurrentmodificationexceptions when there is a loop processing ongoing and at the same time add() or delete() methods are called on the ArrayList. Some code locations try to solve this problem by creating another copy of the ArrayList "new ArrayList(listeners)" but this is nothing but a hack working in some situations somehow with the price of waste of memory cpu time but not in general as the underlying listeners datastructure might change before the loop processing is finished. Again it just hides the underlying problem but does not fix it. The problem 1. has been fixed in https://git.eclipse.org/r/#/c/152786/. Foreach is just another syntax for iterator based loop. However because of this change the problem 2. becomes obvious in bug 558510 because datastructure ArrayList fails early with a concurrentmodificationexception. To solve 2. I created https://git.eclipse.org/r/#/c/154893/ . Unfortunately the broken class seems to be used in a lot of other code and has no clear api so that internal changes affect other modules. One could try to use https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/CopyOnWriteArrayList.html instead of simple ArrayList. That would be compatible with other packages that make use of this class and be basically the same approach as using "new ArrayList(listeners)" before the loop. For the problem when calling delete() and add() on the original ArrayList datastructure while a foreach loop is procesing it does not help - I did not check if something like this is happening in TextViewer.java - other code in eclipse is using such constructs. The right thing there would be to refactor the code to call add() and delete() on the iterator (then you cannot use foreach as you need access to the iterator object) or use a datastructure like you see in https://git.eclipse.org/r/#/c/154893/. My suggestion is to use something like https://git.eclipse.org/r/#/c/154893/ maybe slightly changed to use a concurrent set like https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/ConcurrentSkipListSet.html as this would allow to skip the checking for if the listener is already part of listeners. Because the set is sorted all the time it might be faster as well.
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.
