SPARK-2378: Make listeners thread safe

[stokito]

The ChatContainer class has an iteration over list of chat rooms

        for (ChatRoom chatRoom : new ArrayList<>(chatRoomList)) {
           // ...
        }

The new ArrayList<>(chatRoomList allocation is needed to avoid concurrency problems. The chatRoomList is changed in two places: in the addChatRoom() that is synchronized and cleanupChatRoom() that is not synchronized. To avoid problems I made it synchronized too.

[stokito]

It looks like some pattern or remains after some refactoring because there are other places with such iteration over an ArrayList copy:
org/jivesoftware/spark/SessionManager.java:206
org/jivesoftware/spark/filetransfer/SparkTransferManager.java:695
org/jivesoftware/spark/ui/ChatRoom.java:1139
org/jivesoftware/spark/ui/ContactGroup.java:270
org/jivesoftware/spark/ui/ContactGroup.java:284
org/jivesoftware/spark/ui/ContactGroup.java:416
org/jivesoftware/spark/ui/ContactList.java:379

[Fishbowler]

Does that mean that this should go to draft, or should this be tackled in another PR?

[stokito]

It creates a copy of the list (new ArrayList<>(this.presenceListeners)) to avoid concurrent modification issues if a listener modifies the list during iteration.

I can't replace all the places where this happens but for listeners we have a few options:

• Use Swing's EventListenerList that is just a synchronized list.
• Use CopyOnWriteArrayList and it is recommended https://stackoverflow.com/a/36571559/1049542

The CopyOnWriteArrayList is already used in some places for listeners also an AI recommended to use it.

So I changed all the places where I found a list of listeners to the CopyOnWriteArrayList so that same pattern will be used everywhere. I can't properly test it but Sparks looks working fine.

[guusdk]

Is this fixing an actual problem that someone reported or observed, or is this a theoretical fix? I would very much prefer to have a JIRA ticket for this change, as it seems to be an important change that I'd like to have documented.

[stokito]

@guusdk this is not a bug fix and initially I was just confused why the allocation happens. But in fact there may be threading problems in various places. The PR should make the Spark more stable against them but also the code will be more clean and it will use the same pattern everywhere. Currently in most places is used ArrayList with a copy on iteration, in a few places is used the CopyOnWriteArray, in others the synchronized methods.

I created a ticket for this https://igniterealtime.atlassian.net/browse/SPARK-2378

[guusdk]

Thanks! Can you rewrite the commit messages to include a reference to that JIRA identifier please (and maybe squash many of the very duplicate commits while you're at it)? That will allow automation to detect which commits belong to which tickets, which is very helpful for future developers.

[stokito]

I squashed, added the ticet number and rebased on top of master.