CDV ❯ ReentrantLock.isLocked() returns incorrect value
-
Bug
-
Status: Closed
-
2 Major
-
Resolution: Fixed
-
-
-
cdennis
-
Reporter: hmak
-
August 25, 2008
-
1
-
Watchers: 3
-
July 27, 2012
-
November 16, 2009
Attachments
Description
ReentrantLock.isLocked() returns false [instead of true] if the lock is a DSO and is held by a thread. This problem also occurs for ReentrantReadWriteLock’s.
Reproduce case:
Attached are 2 reproduce cases [originally reported by http://forums.terracotta.org/forums/posts/list/0/1337.page].
Expected output [for both tests] is: test 1: [isLocked=true].isLocked() is ok test 2: [isLocked=true].tryLock() is ok But Terracotta generates: test 1: [isLocked=false].isLocked() is BAD! test 2: [isLocked=false].tryLock() is ok
Possible workaround (not 100%):
Use tryLock()/unlock() to emulate isLocked(), but this won’t work if multiple threads do this simultaneous on a non-locked ReentrantLock. Otherwise, avoid ReentrantLock altogether if isLocked() needs to be accurate.
Comments
Steve Harris 2008-09-19
Geert Bevin 2008-10-01
Here’s a snippet that would make isLocked() fail in the case of having several threads on the same client lock manager since “lock.isHeldBy(threadID, lockLevel);” will check for the currently running thread while asserting “isHeldBy”. The ‘else’ condition MIGHT probably work since it will grab the lock info from the server, the threadID doesn’t seem to make a difference since it returns the lockinfo that is checked through “lockInfo.isLocked(lockLevel);”.
ClientLockManagerImpl:
// TODO: // Needs to take care of the greedy lock case. public boolean isLocked(LockID lockID, ThreadID threadID, int lockLevel) { ClientLock lock; synchronized (this) { waitUntilRunning(); lock = (ClientLock) locksByID.get(lockID); } if (lock != null) { return lock.isHeldBy(threadID, lockLevel); } else { GlobalLockInfo lockInfo = getLockInfo(lockID, threadID); return lockInfo.isLocked(lockLevel); } }
The first code path of this “if” statement will never be checked through the “ReentrantLockTestApp.basicLockTesting” test since only one thread will execute for each node in the test.
Geert Bevin 2008-10-01
Oh and we said that handling the greedy locks in an absolutely correct fashion is probably not needed since isLocked is a race anyway. A greedy lock has at least been held once and you’re never really sure if it’s actually still held or merely in the greedily granted state.
Troy Anderson 2009-04-11
I spent a fair amount of time stumbling over this one. Please fix, even if it is slow, to save the next person a couple of days of pulling their hair out. :)
Chris Dennis 2009-11-16
This was a bug in the old lock manager implementation. I found this by inspection when designing the new lock manager, and both attached tests now work correctly.
Any fix we could do to make the isLocked flag accurate would likely be pretty slow. Would a slow version of isLocked be useful?