Menu
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).
455
Thread safety becomes a concern if there is at least a single entry point which can be accessed by multiple threads. If a piece of code is accessed by multiple threads and is calling other method/class/etc., then all this code tree becomes vulnerable.
Thread safety is the avoidance of data races—situations in which data are set to either correct or incorrect values, depending upon the order in which multiple threads access and modify the data. When no sharing is intended, give each thread a private copy of the data.
Thread-safety violations are a generalization of data races to objects and data structures. Each class or library specifies, sometimes implicitly, a thread-safety contract that determines the set of functions that can be called concurrently by threads in the program.
A method will be thread safe if it uses the synchronized keyword in its declaration. However, even if your setId and getId methods used synchronized keyword, your process of setting the id (if it has not been previously initialized) above is not. .. but even then there is an "it depends" aspect to the question.
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); } If you love us? You can donate to us via Paypal or buy me a coffee so we can maintain and grow! Thank you!
Donate Us With
