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

There are some issues:

1. map increases indefinitely
2. lock should not be called inside the try-final
3. do not create an object if existing key

Now fixing the first issue is not easy - just calling userLock.remove(userId); is not enough:

public class UserManagement {

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

    public void createUser(User user, Integer userId) {
        Lock lock = userLock.computeIfAbsent(userId, k -> new ReentrantLock());
        lock.lock();
        try {
            // do user logic
        } finally {
            lock.unlock();
        }
        // Danger: this could remove the lock although another thread is still inside the 'user logic'
        userLock.remove(userId);
    }
}

To my current knowledge you could fix all problems, save even a bit memory and avoid explicit locking. The only requirement according to the javadocs is that the "user logic" is fast:

// null is forbidden so use the key also as the value to avoid creating additional objects
private final ConcurrentHashMap<Integer, Integer> userLock = ...;

public void createUser(User user, Integer userId) {
  // Call compute if existing or absent key but do so atomically:
  userLock.compute(userId, (key, value) -> {
      // do user logic
      return key;
  });
  userLock.remove(rowIndex);
}

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.

Answer 3

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 4

Looks thread safe as long as you don't create any new threads in the locked logic code block. If you do create threads in the locked logic code block and those threads would call any of the UserManagement methods for the same user then you'd end up with a deadlock.

You also need to ensure that you have only one instance of UserManagement. If you create multiple instances, then you can have multiple threads updating the same user. I suggest making userLock static to avoid that problem.

Just one other minor nitpik with the application logic. When passing in the user, you need to ensure that you don't pass in the same user with different userIds (not sure why you pass in the userId separate from the user object). This requires additional logic outside this class for the creation/import of a new user. Otherwise you could end up calling createUser(userA, 1) and createUser(userA,2) or import(userA,3).