Is Map of Locks a safe approach for concurrent operations

Question

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 1

Your code meets the requirement about safe access to operations on users, but it's not fully thread safe because it doesn't guarantee so called initialization safety. If you create an instance of UserManagement and share it between several threads, those threads can see uninitialized userLock under some circumstances. Though very unlikely, it's still possible.

To make your class fully thread-safe, you need to add final modifier to your userLock, in this case Java Memory Model will guarantee proper initialization of this field in the multi-thread environment. It's also a good practice to make immutable fields final.

Important update: @sdotdi made a point in comments that after the constructor has finished its work, you can fully rely on the internal state of the object. Actually, its not true and things are more complicated.

The link provided in the comment, only covers the early stage of Java code compilation and says nothing about what happens further. But further, optimizations come to play and start reordering instructions as they like. The only restriction the code optimizer has, is JMM. And according to JMM, it's fully legal to change the assignment of the pointer to the new instance of the object with what happened in its constructor. So, nothings stops it from optimizing this pseudo code:

UserManagement _m = allocateNewUserManagement(); // internal variable
_m.userLock = new ConcurrentHashMap<>();
// constructor exits here
UserManagement userManagement = _m;  // original userManagement = new UserManagement()

to this one:

UserManagement _m = allocateNewUserManagement();
UserManagement userManagement = _m;
// weird things might happen here in a multi-thread app
// if you share userManagement between threads
_m.userLock = new ConcurrentHashMap<>();

If you want to prevent such behaviour, you need to use some sort of synchronization: synchronized, volatile or a softer final as in this case.

You can find more details, for instance, in the book 'Java Concurrency in Practice', section 3.5 'Safe publication'.

Answer 2

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.