Java Non Reentrant Lock Implementation

Question

I have implemented a Non Reentrant Lock. I want to know if this has any mistakes, race conditions etc. I am aware of the fact that existing libraries have to be used (instead of writing our own), but this is just to see if I am understanding the java concurrency correctly. Any feedback is appreciated.

public class MyLock {
private boolean isLocked = false;
private long owner = -1;
private static String TAG = "MyLock: ";

public synchronized void Lock() throws InterruptedException, IllegalStateException {
if(owner == Thread.currentThread().getId()) {
    throw new IllegalStateException("Lock already acquired. " +
                                    "This lock is not reentrant");
} else {
    while(isLocked == true) {
        System.out.println(TAG+"Waiting for Lock, Tid = " +
                Thread.currentThread().getId());
        wait();
    }
}

isLocked = true;
owner = Thread.currentThread().getId();           
System.out.println(TAG+"Lock Acquired: Owner = " + owner);
}

public synchronized void Unlock() throws IllegalStateException {
if(!isLocked || owner != Thread.currentThread().getId()) {
    throw new IllegalStateException("Only Owner can Unlock the lock");
} else {
    System.out.println(TAG+"Unlocking: Owner = " + owner);
    owner = -1;
    isLocked = false;
    notify();
}
}

Answer 1

Your code works, but its quality leaves some room for improvement. It should be simplified by refactoring. Here are some suggestions:

Naming

Even if it is only an exercise, utmost care should be applied when choosing names to make sure they are descriptive and clear, but also to comply with coding conventions:

• Consider calling your class NonReentrantLock instead of MyLock.
• Methods should be camel-case, starting with a lower-case character, so Lock() should be lock() and Unlock() should be unlock().
• Only constants should be upper-case, so TAG should be made a constant. But TAG can mean anything. The solution is to refactor your code (as shown below) such that TAG is only used once and is no longer needed as a class variable.

Constants

• TAG is not a useful constant because it leads to lots of System.out.println calls which you want to avoid. All your logging should happen in a single place, not be spread out over your class.

• However, you are using the magic value of -1 to show that nobody owns the lock at the moment. You should introduce a constant NOBODY and use that to set owner:

  private static final long NOBODY = -1;
  private long owner = NOBODY;
  

Redundancy

Tracking the same state twice

Look carefully at your instance variables owner and isLocked. Track their values during runtime with the debugger, and you will see: if owner == -1 then isLocked == false, but if owner != -1 then isLocked == true. In other words, you are using two variable to track the same piece of state.

You should remove isLocked and replace it it with the following method:

private synchronized boolean isLocked() {
    return owner != NOBODY;
}

Doing the same string concatenations multiple times

You have used System.out.println throughout your code, always passing TAG, a message, and a thread id. You can extract this functionality into a new method:

private void log(String message, long threadId) {
    System.out.println(String.format("MyLock: %s %s", message, threadId));
}

That way, System.out.println is only called in a single place and can easily be disabled or replaced with a real logger. For instance, this:

System.out.println(TAG+"Lock Acquired: Owner = " + owner);

can become this:

log("lock acquired, owner =", owner);

Miscellaneous redundancies

• If you throw an exception, inside an if-structure, you the else branch is redundant, so code like

  if (condition) {
      throw new IllegalStateException(...);
  } else {
      doSomethingElse();
  }
  

  can become

  if (condition) {
      throw new IllegalStateException(...);
  }
  doSomethingElse();
  

• You have code duplication when checking if the current thread is the owner of the lock. Refactor out this method instead:

  private boolean ownerIsCurrentThread() {
      return owner == Thread.currentThread().getId();
  }
  

• You should extract argument validation into a separate method:

  private void throwIf(boolean condition, String message) throws IllegalStateException {
      if (condition) {
          throw new IllegalStateException(message);
      }
  }
  

  which will allow you to replace your argument validation if-statements with code like

  throwIf(ownerIsCurrentThread(), "the lock has already been acqired");
  

Error messages

At the beginning of unlock(), you throw an IllegalStateException with the same message for two completely different reasons. That's not very helpful for debugging. Instead, check them separately:

public synchronized void unlock() throws IllegalStateException {
    throwIf(!isLocked(), "only a locked lock can be unlocked");
    throwIf(!ownerIsCurrentThread(), "only owner can unlock the lock");

End Result

public class NonReentrantLock {
    private static final long NOBODY = -1;
    private long owner = NOBODY;

    public synchronized void lock() throws InterruptedException, IllegalStateException {
        throwIf(ownerIsCurrentThread(), "the lock has already been acqired");
        long threadId = Thread.currentThread().getId();
        while (isLocked()) {
            log("waiting for lock on thread", threadId);
            wait();
        }
        owner = threadId;
        log("lock acquired, owner =", owner);
    }

    public synchronized void unlock() throws IllegalStateException {
        throwIf(!isLocked(), "only a locked lock can be unlocked");
        throwIf(!ownerIsCurrentThread(), "only owner can unlock the lock");
        log("unlocking, owner =", owner);
        owner = NOBODY;
        notify();
    }

    private synchronized boolean isLocked() {
        return owner != NOBODY;
    }

    private boolean ownerIsCurrentThread() {
        return owner == Thread.currentThread().getId();
    }

    private void throwIf(boolean condition, String message) throws IllegalStateException {
        if (condition) {
            throw new IllegalStateException(message);
        }
    }

    private void log(String message, long threadId) {
        System.out.println(String.format("MyLock: %s %s", message, threadId));
    }
}

Answer 2

Shouldn't isLocked be set to true somewhere in here? It seems like when this thread gets notified, it will just print "lock acquired" without actually acquiring the lock and setting the owner to itself.

            wait();
        }
        // Perhaps here you set isLocked to true and owner to "me"?
    }
}
System.out.println(TAG+"Lock Acquired: Owner = " + owner);
}

Answer 3

I have read Java Concurrency In Practice and the authors give tips on building custom synchronizers. The AbstractQueuedSynchronizer is the perfect starting point for this purpose. The AQS holds a simple volatile variable integer state which is the 'synchronization state'. The tryAcquire() and tryRelease() are the main methods required to be overridden to implement your Custom Synchronizer. These methods grant the thread exclusive access to the state of the object. The shared and interruptible versions of these methods finally invoke one of these two methods to acquire/release the lock. The compareAndSetState(expectedValue, newValue) is atomic and changes the state of the synchronizer. For a Lock this is from 0 to 1.

import java.util.concurrent.TimeUnit;
import java.util.concurrent.locks.AbstractQueuedSynchronizer;

public class NonRentrantLock {
    private final Sync sync = new Sync();

    public void unlock() {

        sync.release(0);
    }

    public boolean tryLock(int time, TimeUnit timeUnit)
            throws InterruptedException {

        return sync.tryAcquireNanos(0, timeUnit.toNanos(time));
    }

    private static class Sync extends AbstractQueuedSynchronizer {

        /* is the lock already held by some thread ? */
        protected boolean isHeldExclusively() {
            return getState() == 1;
        }

        /*
         * tryAcquire first checks the lock state. If it is unheld, it tries to
         * update the lock state to indicate that it is held.
         */
        protected boolean tryAcquire(int acquires) {

            if (compareAndSetState(0, 1)) {
                setExclusiveOwnerThread(Thread.currentThread());
                return true;
            }
            return false;
        }

        /*
         * tryRelease first checks the lock state. If it is unheld, it throws
         * and IllegalMonitorStateException else it tries to update the lock
         * state to indicate that it is unheld.
         */
        protected boolean tryRelease(int releases) {

            if (getState() == 0)
                throw new IllegalMonitorStateException();
            setExclusiveOwnerThread(null);
            setState(0);
            return true;
        }

    }
}

Answer 4

1. Consider using notifyAll instead of notify:

   A related issue is whether you should use notify or notifyAll to wake waiting threads. (Recall that notify wakes a single waiting thread, assuming such a thread exists, and notifyAll wakes all waiting threads.) It is often said that you should always use notifyAll. This is reasonable, conservative advice. It will always yield correct results because it guarantees that you’ll wake the threads that need to be awakened. You may wake some other threads, too, but this won’t affect the correctness of your program. These threads will check the condition for which they’re waiting and, finding it false, will continue waiting.

   [...]

   From: Effective Java, 2nd Edition, Item 69: Prefer concurrency utilities to wait and notify

2. I'd use a private lock object instead of synchronizing on this with synchronized methods.

   • Avoid synchronized(this) in Java?
   • Java Concurrency in Practice, 4.2.1. The Java Monitor Pattern
   • Item 70: Document thread safety in Effective Java, 2nd Edition also mentions this pattern.

3. A small note to codesparkle's great answer: Instead of the throwIf method I'd use Guava's checkState here. I would prefer this because it's obvious that the method throws IllegalStateException, it's in its name. (It also has checkArgument methods which throw IllegalArgumentException and checkNotNull methods with NullPointerException.)

4. I'd use a logger framework instead of System.out.println statements. I suggest you SLF4J with parameterized messages.