Simple Implementation of a ReentrantLock

Question

An implementation of a ReentrantLock:

public class ReentrantLock {
 private boolean isLocked = false;
 private Thread lockedBy = null;
 private int lockCount = 0;
 public synchronized void lock() throws InterruptedException{
 while(isLocked && Thread.currentThread() != lockedBy){
 this.wait();
 }
 isLocked = true;
 lockedBy = Thread.currentThread();
 lockCount++;
 }
 public synchronized void unlock(){
 if(Thread.currentThread() == lockedBy){
 lockCount--;
 }
 if(lockCount == 0){
 isLocked = false;
 this.notify();
 }
 }
}

Answer 1

Your code looks sensible, with two exceptions, the unlock should only notify when the lock is unlocked.... your code currently allows for asymmetrical notifications (excessive notifications).

Consider:

public synchronized void unlock(){
 if(Thread.currentThread() == lockedBy){
 lockCount--;
 }
 if(lockCount == 0){
 isLocked = false;
 this.notify();
 }
}

The code above notifies even if nothing was locking things. The effect is probably minor, but it is important for classes like this to show symmetry. The code should be:

public synchronized void unlock(){
 if (!isLocked || lockedBy != Thread.currentThread()) {
 return;
 }
 lockCount--;
 if(lockCount == 0){
 isLocked = false;
 this.notify();
 }
}

A further vulnerability is that you leak the lock's monitor. Using synchronized methods is a problem, because someone can simply synchronize on your whole class, and deadlock the entire system.... Consider a thread that maliciously does:

synchronized (lockInstance) {
 Thread.sleep(10000000);
}

Now no other thread can lock, or unlock that instance, and will just hang.

You should instead use a private monitor:

private final Object sync = new Object();

and then synchronize on that:

synchronized (sync) {
 .....
}

• tusharmath: If we check the current thread first and then do the rest of the operations, can't we drop the synchronized keyword completely?

Answer 2

You can also get rid of the isLocked instance variable and replace the expression isLocked with lockedBy != null.

Combining this with @rofl's suggestions, I arrive at the following:

class ReentrantLock {
 private final Object sync = new Object(); // private monitor
 private Thread lockedBy = null; // null => unlocked 
 private int lockCount = 0;
 public void lock() throws InterruptedException {
 synchronized (sync) {
 Thread callingThread = Thread.currentThread();
 while (lockedBy != null && lockedBy != callingThread)
 wait();
 lockedBy = callingThread; // (re)locked! 
 lockCount++;
 }
 }
 public void unlock() {
 synchronized (sync) {
 if (Thread.currentThread() == lockedBy)
 if (--lockCount == 0) {
 lockedBy = null; // unlocked! 
 notify();
 }
 }
 }
}

• IEatBagels: Dumping code isn't considered a review.
• wcochran: @topinfrassi I understand, yet I thought it would be nice to see a complete solution a result of previous comments -- at least that is what I was looking for originally.