R H R H - 3 months ago 19
Java Question

Is Map of Locks a safe approach for concurrent operations

The requirement is only single thread must be allowed to perform user management (create/update/import) operations but multiple threads are not allowed to perform user operations simultaneously for the same user. For example, When Thread A is creating User A then concurrently Thread B must not be allowed to import User A Or Creating User A but Thread B is allowed to import User B. Is the below code thread safe for these requirements?

public class UserManagement {

ConcurrentHashMap<Integer, Lock> userLock = new ConcurrentHashMap<>();

public void createUser(User user, Integer userId) {
Lock lock = userLock.putIfAbsent(userId, new ReentrantLock());
try {
lock.lock();
//create user logic
} finally {
lock.unlock();
}
}

public void importUser(User user, Integer userId) {
Lock lock = userLock.putIfAbsent(userId, new ReentrantLock());
try {
lock.lock();
//import user logic
} finally {
lock.unlock();
}
}

public void updateUser(User user, Integer userId) {
Lock lock = userLock.putIfAbsent(userId, new ReentrantLock());
try {
lock.lock();
// update user logic
} finally {
lock.unlock();
}
}
}

Answer

Your program has another bug besides the one that Andrew Lygin mentioned.

This sets lock to null if userId has not previously been seen, because `putIfAbsent(...) does not return the new value, it returns the previous value:

Lock lock = userLock.putIfAbsent(userId, new ReentrantLock());

Do this instead:

Lock lock = userLock.computeIfAbsent(userId, k -> new ReentrantLock());

computeIfAbsent(...) returns the new value. Also, it has the side benefit of not actually creating a new Lock object unless one actually is needed. (Kudos to @bowmore for suggesting it.)


Is this program thread safe?

Assuming you fix the bugs, We still can't tell about the program. All we can tell is that an instance of your UserManagement class will not allow overlapped calls to any of those three methods for the same userId.

Whether or not that makes your program thread safe depends on how you use it. For example, your code won't allow two threads to update the same userId at the same time, but if they try, it will allow them to go one after the other. Your code won't be able to control which one goes first---the operating system does that.

Your locking likely will prevent the two threads from leaving the user record in an invalid state, but will they leave it in the right state? The answer to that question goes beyond the bounds of the one class that you showed us.

Thread safety is not a composeable property. That is to say, building something entirely out of thread safe classes does not guarantee that the whole thing will be thread-safe.