A mutex that can detect certain types of programming error

Question

This mutex prevents concurrent access to a target resource. One key requirement is the ability to be acquired in a (scheduler) thread and released in a different (worker) thread. Unfortunately this means that widely used primitives like the implicit lock (synchronized) or Lock from java.util.concurrent cannot be used because they do not allow the thread that acquires the lock to pass it around to other threads.

This key requirement may cause programming errors when the mutex is kept in a long-lived variable (or even an instance field) and the method .release() is called more than once. We want such programming errors to throw specific runtime exceptions instead of silently allowing incorrect executions of the program.

Performance is not important. This lock will mostly not be contended by two or more threads. Correctness required, readability valued.

API is taken from Semaphore (this class can be viewed as a Semaphore with just 1 permit that does not allow number of permits to become negative):

• acquire()
• acquire(long timeout, TimeUnit timeUnit)
• tryAcquire()
• tryAcquire(long timeout, TimeUnit timeUnit)
• release(Combination combination)

Usage pattern in client code:

Combination combination = lock.acquire();
threadPool.submit(() -> {
   try { ... } finally {
       lock.release(combination);
   }
});

Please note that every time the lock is acquired a new Combination is generated and returned to the caller. Only a call to release() with the very same Combination can make the lock available again. Once released, the internal combination is cleared (set to null) and the lock is said to be unlocked.

public class CombinationLock {

    // This lock is used to synchronize access to the combination field
    private final Lock lock = new ReentrantLock();
    private final Condition lockReleased = lock.newCondition();

    // Initially null, this field is set to a non-null value from acquire/tryAcquire methods
    // and then set to null again from release().
    private Combination combination;

    /**
     * Block the caller thread indefinitely until the lock is acquired
     *
     * @return the combination for releasing the lock
     * @throws InterruptedException if waiting thread is interrupted
     */
    public Combination acquire() throws InterruptedException {
        lock.lockInterruptibly();
        try {
            while (combination != null) {
                lockReleased.await();
            }
            combination = Combination.generate();
            return combination;
        } finally {
            lock.unlock();
        }
    }

    /**
     * Wait for the lock to be available. If available at the time of invocation, lock and return the
     * combination to release it later, otherwise just return null.
     *
     * @return null, or the combination for releasing the lock
     */
    public Combination tryAcquire() throws InterruptedException {
        boolean locked = lock.tryLock();
        if (locked) {
            try {
                if (combination == null) {
                    combination = Combination.generate();
                    return combination;
                } else {
                    return null;
                }
            } finally {
                lock.unlock();
            }
        } else {
            return null;
        }
    }

    /**
     * Wait for the given timeout, otherwise return null
     *
     * @param timeout in arbitrary unit
     * @param unit the timeout unit
     * @return NULL, of the combination to release the lock
     * @throws InterruptedException if thread is interrupted
     */
    public Combination tryAcquire(long timeout, TimeUnit unit) throws InterruptedException {
        Stopwatch stopwatch =  Stopwatch.createStarted();
        boolean locked = lock.tryLock(timeout, unit);
        if (locked) {
            try {
                while (combination != null) {
                    long remainingWaitTimeNanos = unit.toNanos(timeout) - stopwatch.elapsed(TimeUnit.NANOSECONDS);
                    boolean timeRunOut = lockReleased.await(remainingWaitTimeNanos, TimeUnit.NANOSECONDS);
                    if (timeRunOut) {
                        return null;
                    }
                }
                combination = Combination.generate();
                return combination;
            } finally {
                lock.unlock();
            }
        } else {
            return null;
        }
    }

    public boolean isLocked() {
        lock.lock();
        try {
            return combination != null;
        } finally {
            lock.unlock();
        }
    }

    /**
     * Release the lock
     *
     * @param combination previously generated from acquire/tryAcquire
     */
    public void release(Combination combination) {
        Preconditions.checkNotNull(combination, "Given combination must be non-null. Did you forget to check [combination != null] after calling tryAcquire()?");

        lock.lock();
        try {
            if (!isLocked()) {
                throw new RuntimeException("This lock is not locked");
            }

            if (this.combination.equals(combination)) {
                this.combination = null;
                lockReleased.signalAll();
            } else {
                throw new RuntimeException("Someone else acquired the lock since you got your combination so you must have released it");
            }
        } finally {
            lock.unlock();
        }
    }
}

Answer 1

Welcome, concurrency is hard! Have you tested this code? Do you have unit tests?

• Also public isLocked method seems useless. What is the point of it and it's lock? If you acquire lock for such a short time (just to get the boolean, the information from the method is useless, because it may be outdated very fast. And if you want to react to it, you want to do it in atomic way. The alternative is to remove the lock from it, because it doesn't really do anything useful for you in this case.

• I would be careful with returning nulls (even your sample usage can throw NullPointerException when acquire fails!). In your acquire methods, consider using Optional or throwing exception instead.

• To ensure the lock is always locked or unlocked, less error-prone is to have some basic skeleton of a method, that does this for you and then in the "middle" executes your functional block. Then your atomic critical section are those lambdas and they are separated from synchronization code, which is in one place. The code is cleaner and less error-prone.

• I see no point of Combination class existence. Is it to make sure that you don't release incorrect CombinationLock instance?