How much thread-safety is too much?

Question

I've been reading Java Concurrency in Practice lately – great book. If you think you know how concurrency works, but then most of the time you face the real issues, it feels like SWAG is the most you can do, then this book will certainly shed some light on the topic. It's sort of scary how many things can actually go wrong when you try to share data between threads. I guess that made me probably a bit crazy about thread-safety. Now my concern is that, with a bit too much synchronization, I may run into some liveness issues. Here's a piece of code to illustrate:

   private final Hashtable<String, AtomicInteger> userSessions =
new Hashtable<String, AtomicInteger>();

   public void registerUser(String userLogin) {
       synchronized(userSessions) {
           AtomicInteger sessionCount = userSessions.get(userLogin);
           if (sessionCount != null) {
               sessionCount.incrementAndGet();
           } else {
               userSessions.put(userLogin, new AtomicInteger(1));
           }
       }
   }

   public void unregisterUser(String userLogin) {
       synchronized(userSessions) {
           AtomicInteger sessionCount = userSessions.get(userLogin);
           if (sessionCount != null) {
               sessionCount.decrementAndGet();
           }
       }
   }

   public boolean isUserRegistered(String userLogin) {
       synchronized(userSessions) {
           AtomicInteger sessionCount = userSessions.get(userLogin);
           if (sessionCount == null) {
               return false;
           }
           return sessionCount.intValue() > 0;
       }
   }

I tried getting it all right: synchronized collection constructed in static section and stored in a static final reference for safe publication, locking on the collection (instead of this - so that I don't block the whole class the code lives in) and using atomic wrapper classes for primitives. The book mentions overdoing this might also cause problems, but it seems I need some more time to fully wrap my head around it. How would you make this code thread-safe and make sure it doesn't suffer from liveness and also performance issues?

EDIT: Turned it into instance methods and variables, originally everything was declared as static - bad, bad design. Also made userSessions private (somehow I left it public before).

Answer 1

Use a ConcurrentHashMap so that you can use putIfAbsent. You don't need to AtomicInteger code to be synchronised.

   public final ConcurrentMap<String, AtomicInteger> userSessions =
       new ConcurrentHashMap<String, AtomicInteger>();

   public void registerUser(String userLogin) {
       AtomicInteger newCount = new AtomicInteger(1);
       AtomicInteger oldCount = userSessions.putIfAbsent(userLogin, newCount);
       if (oldCount != null) {
           oldCount.incrementAndGet();
       }
   }

   public void unregisterUser(String userLogin) {
       AtomicInteger sessionCount = userSessions.get(userLogin);
       if (sessionCount != null) {
           sessionCount.decrementAndGet();
       }
   }

   public boolean isUserRegistered(String userLogin) {
       AtomicInteger sessionCount = userSessions.get(userLogin);
       return sessionCount != null && sessionCount.intValue() > 0;
   }

Note, this leaks...

Attempt at a non-leaky version:

   public final ConcurrentMap<String, Integer> userSessions =
       new ConcurrentHashMap<String, Integer>();

   public void registerUser(String userLogin) {
       for (;;) {
           Integer old = userSessions.get(userLogin);
           if (userSessions.replace(userLogin, old, old==null ? 1 : (old+1)) {
                break;
           }
       }
   }
   public void unregisterUser(String userLogin) {
       for (;;) {
           Integer old = userSessions.get(userLogin);
           if (old == null) {
               // Wasn't registered - nothing to do.
               break;
           } else if (old == 1) {
               // Last one - attempt removal.
               if (userSessions.remove(userLogin, old)) {
                   break;
               }
           } else {
               // Many - attempt decrement.
               if (userSessions.replace(userLogin, old, old-1) {
                   break;
               } 
           }
       }
   }
   public boolean isUserRegistered(String userLogin) {serLogin);
       return userSessions.containsKey(userLogin);
   }

Answer 2

First of all: Don't use the Hashtable! It's old, and very slow.
Additional: Synchronisation on the lower level isn't needed if you synchronize on a higher level already (This is true for the AtomicInteger-thing, as well).

I see different approaches here, according to what use case is needed here.

The read/write-approach

Assuming you call the isUserRegistered method very often and the other methods only now and then, a good way is a read-write-lock: It is allowed to have multiple reads at the same time, but only one write-lock to rule them all (can only be gained if no other lock is accquired).

private static final Map<String, Integer> _userSessions =
  new HashMap<String, Integer>();

private ReadWriteLock rwLock =
  new ReentrantReadWriteLock(false); //true for fair locks

public static void registerUser(String userLogin) {
  Lock write = rwLock.writeLock();
  write.lock();
  try {
       Integer sessionCount = _userSessions.get(userLogin);
       if (sessionCount != null) {
           sessionCount = Integer.valueOf(sessionCount.inValue()+1);
       } else {
           sessionCount = Integer.valueOf(1)
       }
       _userSessions.put(userLogin, sessionCount);
   } finally {
     write.unlock();
   }
}

public static void unregisterUser(String userLogin) {
  Lock write = rwLock.writeLock();
  write.lock();
  try {
       Integer sessionCount = _userSessions.get(userLogin);
       if (sessionCount != null) {
           sessionCount = Integer.valueOf(sessionCount.inValue()-1);
       } else {
           sessionCount = Integer.valueOf(0)
       }
       _userSessions.put(userLogin, sessionCount);
   } finally {
     write.unlock();
   }
}

public static boolean isUserRegistered(String userLogin) {
  boolean result;

  Lock read = rwLock.readLock();
  read.lock();
  try {
       Integer sessionCount = _userSessions.get(userLogin);
       if (sessionCount != null) {
           result = sessionCount.intValue()>0
       } else {
           result = false;
       }
   } finally {
     read.unlock();
   }

   return false;
}

Pro: simple to understand
Con: Won't scale if the write methods are called frequently

The small atomic-operation approach

The Idea is to do small steps, that are all atomic. This will lead to a very good performance anyway, but there are many hidden traps here.

public final ConcurrentMap<String, AtomicInteger> userSessions =
   new ConcurrentHashMap<String, AtomicInteger>();
//There are other concurrent Maps for different use cases

public void registerUser(String userLogin) {
  AtomicInteger count;
  if (!userSession.containsKey(userLogin)){
    AtomicInteger newCount = new AtomicInteger(0);
    count = userSessions.putIfAbsent(userLogin, newCount);
    if (count == null){
      count=newCount;
    }
    //We need ifAbsent here, because another thread could have added it in the meantime
  } else {
    count = userSessions.get(userLogin);
  }
  count.incrementAndGet();
}

public void unregisterUser(String userLogin) {
  AtomicInteger sessionCount = userSessions.get(userLogin);
  if (sessionCount != null) {
    sessionCount.decrementAndGet();
  }
}

public boolean isUserRegistered(String userLogin) {
  AtomicInteger sessionCount = userSessions.get(userLogin);
  return sessionCount != null && sessionCount.intValue() > 0;
}

Pro: scales very well
Con: Not intuitive, is going to be complex quickly, not always possible, many hidden traps

The lock per user approach

This will create locks for different users, assuming that there are a lot of different users. You can create locks or monitors with some small atomic operations and lock on these instead of the full list.
It would be overkill for this small example, but for very complex structures it can be an elegant solution.

Answer 3

Seen from your code, your synchronisation on _userSessions should suffice because you do not expose the AtomicInteger objects.

The added safety offered by AtomicInteger is not needed in this case, so in essence you use it here as a mutable Integer. You could place a nested static class containing the session count as only attribute in the map if you are worried about the extra overhead in AtomicInteger (or a bit uglier: add int[1]'s to the map as long as they aren't exposed outside of this class.)

Answer 4

Good book, I recently read it myself.

In the code above, the only note I have is that AtomicInteger isn't needed within the synchronized block, but I doubt the performance would be noticeable.

The best way to track performance is to test it. Set up an automated integrated load test around key areas of your framework and track performance. The load test, if it contains wide enough timing windows and a rich use of the work flow, may also catch any deadlocks you've created.

While deadlocks may seem easy to avoid, they can easily appear in a fairly simple workflow pattern.

Class A locks resources then calls B (may as simple as a get/set) which also locks resource. Another thread calls B which locks resources and then calls A causing a deadlock.

When working with a rich framework it is useful to map out workflow to see how classes interact. You may be able to spot this type of problem. However, with really large frameworks, they can slip by. The best defense I've found is to isolate locks to the smallest area possible and be very conscious of a call outside of a class while within a synchronized block. Create a significant number of load tests helps as well.

Answer 5

I came across this years-old question looking for advice about how to make what one might call a "concurrent counting map" - searching in particular for uses of ConcurrentHashMap with AtomicInteger.

Here's a modified version of the highest-rated answer which uses AtomicInteger and doesn't leak. In my (limited) testing this seems much faster than the Integer version. I'll also note that using ConcurrentMap.get() before ConcurrentMap.putIfAbsent() does seem to save noticeable time.

private final ConcurrentMap<String, AtomicInteger> userSessions =
   new ConcurrentHashMap<String, AtomicInteger>();

public void registerUser(String userLogin) {
    AtomicInteger oldCount = userSessions.get(key);
    if(oldCount!=null && getAndIncrementIfNonZero(oldCount)>0) return;
    AtomicInteger newCount = new AtomicInteger(1);
    while(true) {
        oldCount = userSessions.putIfAbsent(key, newCount);
        if(oldCount==null) return;
        if(getAndIncrementIfNonZero(oldCount)>0) return;
    }
}

public void unregisterUser(String userLogin) {
   AtomicInteger sessionCount = userSessions.get(userLogin);
   if (sessionCount != null) {
       int endingCount = sessionCount.decrementAndGet();
       if(endingCount==0) userSessions.remove(userLogin);
   }
}

public boolean isUserRegistered(String userLogin) {
   AtomicInteger sessionCount = userSessions.get(userLogin);
   return sessionCount != null && sessionCount.intValue() > 0;
}

private static int getAndIncrementIfNonZero(AtomicInteger ai) {
    while(true) {
        int current = ai.get();
        if(current==0) return 0;
        if(ai.compareAndSet(current, current+1)) return current;
    }
}

Speed may not be so relevant to the original poster's problem, but other applications of such a "counting map" may benefit from efficiency.