Java Synchronization problem — Chatting multiple chatroom applications

Question

I am developing a chat application. I have a function which processes chat messages. Each chatroom is identified by unique shortcode. Now I want that when message for one shortcode is being processed, another message for same shortcode should wait, while messages for other shortcodes should carry on.

Please consider following peace of code, what is the problem with it, as messages for same shortcode are being processed in parallel. I just can't guess the problem

private HashMap<String, Object> locks = new HashMap<String, Object>();

public void handleShortCode(String shortcode,String message,String from)
{
    Object lock = null;

    lock = locks.get(shortcode);
    if (lock == null)
    {
        locks.put(shortcode, lock = new Object());
    }

    synchronized (lock)
    {
        System.out.println("Handling shortcode:" + shortcode);
        // processing
        System.out.println("Successfully handled shortcode:" + shortcode + " ......");
    }
}

Answer 1

First reason I can see is here

Object lock = null;

lock = locks.get(shortcode);
if (lock == null)
{
    locks.put(shortcode, lock = new Object());
}

This block of code executed outside any mutex, so several threads can run it at same time, so each of the threads (having same shortcode) got it's own lock, independent of each other (and only one of them will be stored in locks hashmap -- which one it's the question, since ordinary HashMap not designed for concurrent use -- you can't predict exactly which "put" will actualy takes effect in which order, you can even got exception or wrong behavior in this code, if current puts cause resize for example). Since each thread gets it's own lock, it does not prevent it from grabbing it in concurrent with other threads, getting another lock.

Simplest (but not very efficient) way to fix it:

private HashMap<String, Object> locks = new HashMap<String, Object>();
private final Object hashmapLock = new Object();
public void handleShortCode(String shortcode,String message,String from)
{
    Object lock = null;
    synchronized(hashmapLock){
      lock = locks.get(shortcode);
      if (lock == null)
      {
          locks.put(shortcode, lock = new Object());
      }
    }
    synchronized (lock)
    {
        System.out.println("Handling shortcode:" + shortcode);
        // processing
        System.out.println("Successfully handled shortcode:" + shortcode + " ......");
    }
}

This way you get exactly one lock per shortcode. The more efficient way is to use something like ComputableConcurrentHashMap from Guava lib.

Answer 2

The synchronized mechanism is fine, but maybe you'd like to have a look at Lock interface in java.util.concurrent.locks package. These are more verbose to use, but allows for greater flexibility since it is possible to do things like try and fail, or try to acquire with a timeout.