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

Looks to be thread-safe, but an odd design. How could another thread be doing something to a `User` if it's just being created? — Kayaman, Aug 12 '16 at 06:10
I'm voting to close this question as off-topic because it is a request for a code review — Raedwald, Aug 12 '16 at 08:25
@Raedwald, I would suggest reformulating it to something like 'Is Map of Locks a safe approach for concurrent operations', because the question is interesting per se and it would be nice to find and share the answer. — Andrew Lygin, Aug 12 '16 at 08:38
The library Javadoc says that `lock.lock()` potentially could fail and throw an exception. If that happens, you don't want to call `lock.unlock()`. Your `lock.lock()` call should appear _before_ the `try` keyword. — Solomon Slow, Aug 12 '16 at 09:40
Each of your three methods creates a new `ReentrantLock` instance on every call regardless of whether it's needed or not. That's harmless, but it might cause some developers to raise an eyebrow when they read your code. ---- As I think about that further, it seems like your way is cleaner (and might be faster) than doing the extra work necessary to prevent the construction of un-necessary locks. Only puzzle is how to stop other developers from wanting to "fix" your code. Maybe you should start a movement to name it as a design pattern. — Solomon Slow, Aug 12 '16 at 09:43
@jameslarge Java 8 already has the solution for this : computeIfAbsent : https://docs.oracle.com/javase/8/docs/api/java/util/Map.html#computeIfAbsent-K-java.util.function.Function- — bowmore, Aug 12 '16 at 12:42
How do you safely clean up the map `userLock`? As it currently increases indefinitely. See also my very similar question http://stackoverflow.com/q/39675003/194609 — Karussell, Sep 24 '16 at 11:12

Andrew Lygin · Answer 1 · 2016-08-12T09:37:20.117

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'.

I don't think you are right. Or, to be more precise, I am quite sure you are wrong. If somebody go a hold of `UserManagement` using `new`, it means that the constructor completed. According to the spec http://docs.oracle.com/javase/specs/jls/se8/html/jls-12.html#jls-12.5 first, during construction of a new object, it calls super(), then it initialises instance variables, such as the `userLock` map, then runs the rest of the constructor. So if somebody has obtained a reference to such an object, the `userLock` will be fully initialised. — Dave, Aug 12 '16 at 06:59
I have actually picked up JCiP and read section 3.5. I apologize, it seems I was wrong. I am also a bit disappointed that I cannot rely on the specification. They could at least point towards such publication issues in the section that I linked to. — Dave, Aug 12 '16 at 10:21
@sdotdi, don't be disappointed with the spec, it's absolutely correct and reliable, but it's just hard to grasp all its parts and it has its limits since it's just a language, not the whole platform, specification. Actually, it has a section on Memory Model that explains reordering at the very beginning: http://docs.oracle.com/javase/specs/jls/se8/html/jls-17.html#jls-17.4. — Andrew Lygin, Aug 12 '16 at 11:35
@sdotdi, But then comes JVM with its own spec, comiplers and optimizers and things become even more obscure, let alone the hardware level. JCiP is a very good (though sometimes a bit outdated) guide in this merciless world of Java concurrency, don't throw it away after reading only section 3.5 :) — Andrew Lygin, Aug 12 '16 at 11:36

Solomon Slow · Accepted Answer · 2016-08-12T13:19:38.597

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.

Thanks James for the answer. computeIfAbsent is added in jdk1.8 but what could be a solution for jdk1.7 — R H, Aug 12 '16 at 15:00

Guenther · Answer 3 · 2016-08-12T06:34:55.257

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).

score 0 · Answer 4 · answered Sep 24 '16 at 13:40

There are some issues:

map increases indefinitely
lock should not be called inside the try-final
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);
}

It looks one could even omit removing via returning `null` instead of `return key;` http://stackoverflow.com/a/39676158/194609 — Karussell, Sep 24 '16 at 13:46

Is Map of Locks a safe approach for concurrent operations

4 Answers4

Linked