How does a synchronized statement work in this case?

Question

Assume I have a class like this:

public class Server {

   public static void main(String[] args) {

      Map<Integer, ServerThread> registry = Collections.synchronizedMap(new LinkedHashMap<Integer, ServerThread>());

      ...

      while(true) {
         Socket socket = serverSocket.accept();
         ServerThread serverThread = new ServerThread(id, registry);
         registry.put(id, serverThread);
      }
   }
}

Then:

public class ServerThread extends Thread {

   private Map<Integer, ServerThread> registry;
   private int id;

   public ServerThread(int id, Map<Integer, ServerThread> registry) {
      this.id = id;
      this.registry = registry;
   }

   ...

   private void notify() {
      synchronized(registry) {
         for(ServerThread serverThread : registry.values()) {
            serverThread.callSomePublicMethodOnThread();
         }
      }      
   }
}

I just want to make sure that registry doesn't get modified while I am iterating over it. Does making it a synchronized map guarantee this behavior? Or do I need the synchronized statement. Will the synchronized statement behave like I expect it to?

Thanks

Answer 1

You need the synchronized block around the loop.

See the JavaDoc for the details.

Answer 2

Yes the synchronized statement you have will work like you expect it. I would just add one comment, the thread you are accepting socket connections will block on registry.put(id, serverThread); while you are in the synchronized section on another thread. This means that your server will not process any new incoming requests while you are processing the notification.....

You might want to consider moving the put statement(of course changing serverThread to this) to the very first line of the run method of ServerThread's run method. That way you will not block incoming connections if callSomePublicMethodOnThread winds up taking a long time to process.

Answer 3

To make everything easier I would use ConcurrentHashMap (http://download.oracle.com/javase/1.5.0/docs/api/java/util/concurrent/ConcurrentHashMap.html) so you don't need to use the sync block in the loop, because concurrentHashMap uses a different type of iterator (not fail-fast iterator) and it will not thorw concurrentModificationException, you will also have better performance.