java concurrent Array List access

Question

I have an object which is a singleton. This object declares:

List<Player> players = new ArrayList<Player>();

The same object also specifies 4 operations on this arrayList:

public List<Player> getPlayers() {
return players;
} // the result of this method can be used in another object as an iterator (or maybe by index-access)

and

public void removePlayer(Player player) {
players.remove(player);
}

public void addPlayer(Player player) {
players.add(player);
}

public boolean isPresent(Player player) {
if (players.constans(player)) {...
}

Right now in the constructor I am doing it like that:

players = Collections.synchronizedList(new ArrayList<Player>());

But what is the CORRECT way to synchronize these methods. It seems like if I use iterator in another class, it will still through the concurrent modification exception. Does the exception happen if a 2 threads call at the same time the "remove" and "contains" method? There are many threads to access the singleton so I would like to know the method to do this with the minimum hit on performance.

Answer 1

The documentation answers your question.

It is imperative that the user manually synchronize on the returned list when iterating over it:

List list = Collections.synchronizedList(new ArrayList());
      ...
  synchronized(list) {
      Iterator i = list.iterator(); // Must be in synchronized block
      while (i.hasNext())
          foo(i.next());
  }

As for contains and remove, you shouldn't have to synchronize manually. I'm looking at the source code of Collections and it looks like it does that for you:

    public boolean contains(Object o) {
        synchronized (mutex) {return c.contains(o);}
    }
    public boolean remove(Object o) {
        synchronized (mutex) {return c.remove(o);}
    }

It wouldn't be a synchronized list if you have to do this stuff on your own.

Answer 2

If you don't plan on updating it often use a CopyOnWriteArrayList otherwise @tieTYT's suggestion works.

You can also manage this yourself by returning a copy of the list instead of the actual list within the getPlayers. (If you are using Collections.synchronizedList)

public List<Player> getPlayers(){
    synchronized(list){
       Collections.unmodifiableList(new ArrayList<Player>(list));
    }
}

This has the side effect of a list being out of date after an add/remove is done

Edit: Stealing Grzegorz suggestion for unmodifiableList