-1
\$\begingroup\$

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();
        }
    }
}
\$\endgroup\$
11
\$\begingroup\$

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) {
   .....
}
\$\endgroup\$
-1
\$\begingroup\$

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();
                }
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ Dumping code isn't considered a review. \$\endgroup\$ – IEatBagels Nov 2 '15 at 18:50
  • \$\begingroup\$ @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. \$\endgroup\$ – wcochran Nov 2 '15 at 18:54

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.