ConcurrentHashMap putIfAbsent first time

Question

I have a ConcurrentHashMap and a method that puts a String in the map, then I do some actions in a synchronized block based on the inserted value.

putIfAbsent returns the previous value associated with the specified key, or null if there was no mapping for the key - based on the official documentation

There are 2 actions that are executed based on whether putIfAbsent returns null or not.

Now here is the trick. I want the first action (when putIfAbsent returns null) to be executed first and all other threads to be put on hold. My code works as intended 95% of the time.

private final ConcurrentHashMap<String, String> logins = new ConcurrentHashMap<>();

public void login(String id){
      String inserted=logins.putIfAbsent(id,id);

      synchronized(logins.get(id)){
           if(inserted==null){
                System.out.println("First login");
           }else{
                System.out.println("Second login");
           }           
      }
}

If I call this method with the same String value from different threads login("some_id"); sometimes (around 5% of the time) I get this message on console:

Second login
First login

What do I need to change to always be sure that First login is executed first?

Update: From what I read is it possible that logins.get(id) return null , therefore synchronizing on a null object?

Answer 1

private final ConcurrentHashMap<String, String> logins= new ConcurrentHashMap<>();
private ConcurrentHashMap<String, Object> locks= new ConcurrentHashMap<>();


public void login(String id){

locks.putIfAbsent(id,new Object());
Object lock = locks.get(id);
synchronized(lock)
{
      String inserted=logins.putIfAbsent(id,id);

           if(inserted==null){
                System.out.println("First login");
           }else{
                System.out.println("Second login");
           }           

}
}

Note: Also make sure that you remove the entries from hashmaps when the id is deleted

or Use some other field(apart from string id) to synchronize the code

Answer 2

sometimes (around 5% of the time) I get this message on console:

You have a race condition that the first to add is not the first to print.

In this case, your main bottleneck is the using System.out, this is a contented resource which is many times more expensive than using a Map, concurrent or otherwise.

In which case, you may as well simplify your code so you are only obtaining one lock which is the lock on System.out which you have to obtain anyway

// use System.out as lock so logging of actions is always in order.
private final Set<String> ids = Collections.newSetFromMap(new HashMap<>());

public void login(String id) {
    synchronized (System.out) {
        System.out.println(ids.add(id) ? "First login" : "Second login")l
    }
}

Answer 3

Java offers other synchronization mechanisms that offer more granularity, and IMO, clarity.

Consider the code below. The code illustrates (a) how to protect multiple operations with a lock (b) how the then and else sections can be handled differently (e.g. then protects functions with the lock; else assumes that functions do not require protection. Alter as appropriate for your situation):

class Task implements Runnable {
    private String id;
    private ConcurrentHashMap<String,String> logins;
    private Lock lock;

    public Task(String id, ConcurrentHashMap<String,String> logins, Lock lock) {
        this.id = id;
        this.logins = logins;
        this.lock = lock;
    }

    public void run() {
        login(id);
    }

    public void login(String id){
        lock.lock();

        String inserted = logins.putIfAbsent(id,id);

        if (inserted==null) {
            System.out.print("First login ");
            // other functions that require synchronization
            lock.unlock();
        } else {
            lock.unlock();
            // functions that do NOT require synchronization
            System.out.print("Second login ");
        }           
    }
}