[FIXED] ReentrantLock - should we check who holds the lock while unlocking in finally block?

Issue

A simple snippet get stuck often when delay is slightly high:

public String getToken(String userId, ReentrantLock lock) throws InterruptedException {
    try {
        // lock is in a cache of ConcurrentHashMap with key of userId
        if (!lock.tryLock(LOCK_TIMEOUT_SEC, TimeUnit.SECONDS)) {
            throw new AppException("Could not get lock in " + LOCK_TIMEOUT_SEC + "s"); // 10 sec
        }
        String token = fetchNewToken(userId); // call external endpoint
        lock.unlock();
        return token;
    } finally {
        while (lock.isHeldByCurrentThread()) {
            lock.unlock();
        }
    }

Now the situation is it often locks 10 seconds when fetchNewToken has slightly higher delay(like 1 sec) and throws a lot of exceptions. Formerly it unlocks without checking, I changed the finally part but now I am not sure.

Should I check the lock owner in the finally block? If other threads are holding it, I guess I should not unlock?

Examples I found never checks the owner. So the doubt.

Solution

If other threads are holding it, I guess I should not unlock?

Definitely not.

You're overcomplicating it. Don't unlock unless you know you've acquired it. Move the try-finally block. You don't need 2 calls to unlock either. The finally block will do it for all cases; that's why it exists.

public String getToken(String userId, ReentrantLock lock) throws InterruptedException {
    // lock is in a cache of ConcurrentHashMap with key of userId
    if (!lock.tryLock(LOCK_TIMEOUT_SEC, TimeUnit.SECONDS)) {
        throw new AppException("Could not get lock in " + LOCK_TIMEOUT_SEC + "s"); // 10 sec
    }
    try {
        return fetchNewToken(userId); // call external endpoint
    } finally {
        lock.unlock();
    }

Answered By - Michael
Answer Checked By - Willingham (JavaFixing Volunteer)