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);    }