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

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