java - RMI and Synchronization

Question

I'm currently writing a simple chat application in Java. It has a server application and a client application, both written in Java and use RMI to communicate.

On the server, there is an ArrayList of active users which is updated when users go on and offline. Each client connection to the server has it's own thread.

When a new connection is made (i.e. a new client joins), a new thread is created and within this thread the ArrayList is updated to reflect the new user. Equally when the connection is lost, the ArrayList is updated to reflect the disconnection.

Obviously, with lots of clients there will be lots of concurrent access to the ArrayList. My question therefore, is should any of the methods that access/update this ArrayList be synchronized?

Answer 1

Use Collections.synchronizedList(new ArrayList<User>());

Secondly you can also do

volatile List<User> list = Collections.synchronizedList(new ArrayList<User>());

for guaranteed visibility among threads.

Note : I would also suggest to use Set instead of List to avoid duplication in case of any Errors or corner cases. See the following line of code for reference.

volatile Set<User> onlineUsers = Collections.synchronizedSet(new HashSet<User>());

Answer 2

The other answers have suggested using Collections.synchronizedList().

List<User> list = Collections.synchronizedList(new ArrayList<>());

This might work, but you have to be extremely careful. Such a list is only thread-safe for individual method calls directly on that list. However, many operations involve multiple operations on the list, such as iterating over the list. For example, one might write the following code to send a message to every user in the chat:

for (User u : list)
    u.sendMessage(msg);

This will fail with a ConcurrentModificationException if a user is added to or removed from the list during the iteration. This occurs because this form of the for-loop gets an Iterator from the list and makes repeated accesses to the list via the hasNext and next calls. The list can be modified between these accesses. If the list is modified, the Iterator detects this and throws the exception.

In Java SE 8, one could instead write the following:

list.forEach(u -> u.sendMesage(msg));

This is safe, because the list's lock is held for the entire duration of the forEach call.

(Another suggestion was to use a Set instead of a List. This might be a good idea for other reasons, but it suffers from the same concurrency issues as List.)

Probably the best way to deal with this is to create your own object that contains a list (or set) of users and any related data, and defines a specific set of operations on your container of users and related data. Then, add proper synchronization around your container object's methods. This allows thread-safe updates to the other data besides just your list or set of users.

Finally, volatile is not necessary if the list field is initialized at construction time, before the object is made visible to other threads. In RMI terms, if construction is complete before the objects are exported, you're safe. It's good practice to make the field final so that after initialization, the initialized value is known to be visible to all other threads and cannot change thereafter.

Answer 3

You should use a Collections.synchronizedList() if you want to work with the lists in multiple threads.

This suggestion was already given here.