Will this method work thread-safe and without Deadlocks

Question

public int saveUserToMap(User user) {
    ReentrantLock lock;
    if(this.userLocks.containsKey(user.getId())) {
        lock = this.userLocks.get(user.getId());
    } else {
        lock = new ReentrantLock();
        ReentrantLock check = this.userLocks.putIfAbsent(user.getId(), lock);
        if(check != null)
            lock = check;
    }

    if(lock.isLocked())
        try {
            lock.wait();
        } catch (InterruptedException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
        }

    lock.lock();

    this.users.put(user.getId(), user);
    this.usersByName.put(user.getUsername(), user);
    this.usersByEmail.put(user.getEmail(), user);

    lock.unlock();
    lock.notify();

    return user.getId();
}

Hey, I just want to ask the java developers to check my code if it will be thread-safe and free of Deadlocks as I want to use it in my project. Users, UsersByName and UsersByEmail are ConcurrentHashMap with String, Integer as key and User object as Value. UserLocks is a ConcurrentHashMap with Integer (obviously the user id as key) and a ReentrantLock as value. I want to synchronize the three HashMaps. If someone has a better solution to make a Concurrent map with three keys, it would be nice to post it here. Performance is also important.

Answer 1

It's thread safe.

If the userId is already in the map, the code gets the lock and use it to synchronize. If not, ConcurrentHashMap provides the syncronization to avoid a race condition to use different locks for the same id.

Afterthat, there is a useless fragment of code, you can get rid off:

if(lock.isLocked())
    try {
        lock.wait();
    } catch (InterruptedException e) {
        // TODO Auto-generated catch block
        e.printStackTrace();
    }

It's not needed because the synchronization is being done using lock.lock(). It's not needed to try to synchronize again using wait() and notify() with the lock object.(Actually, it is not working as you expected, several threads can call lock.isLocked() on the same lock object and get a false until any of the threads calls lock.lock(), but everything between lock and unlock is only being executed by a single Thread at a time).

Also, an usual good practice is to call the lock.unlock() in a finally block.

Answer 2

I would do it the simple way using synchronized.

class UserMaps {
    private Map<Integer, User> users = new ConcurrentHashMap<>();
    private Map<String, User> usersByName = new ConcurrentHashMap<>();
    private Map<String, User> usersByEmail = new ConcurrentHashMap<>();

    public synchronized int put(User user) {
        users.put(user.getId(), user);
        usersByName.put(user.getUsername(), user);
        usersByEmail.put(user.getEmail(), user);
        return user.getId();
    }
}

This would ensure that all maps are updated consistently so long as all of your getters are also synchronized.

If, however, you want better performance and want to avoid making all of your methods synchronized then use a ReadWriteLock.

class UserMaps {
    ReentrantReadWriteLock lock = new ReentrantReadWriteLock();
    private Map<Integer, User> users = new ConcurrentHashMap<>();
    private Map<String, User> usersByName = new ConcurrentHashMap<>();
    private Map<String, User> usersByEmail = new ConcurrentHashMap<>();

    public int saveUserToMap(User user) {
        lock.writeLock().lock();
        try {
            users.put(user.getId(), user);
            usersByName.put(user.getUsername(), user);
            usersByEmail.put(user.getEmail(), user);
            return user.getId();
        } finally {
            lock.writeLock().unlock();
        }
    }

    public User getById(int id) {
        lock.readLock().lock();
        try {
            return users.get(id);
        } finally {
            lock.readLock().unlock();
        }
    }
}