2
\$\begingroup\$

I found myself in need of a lock that allows my server to barge the write lock that might be acquired by my users. It would never interrupt them, but it would jump the line so the next time the write lock is available, my server gets it instead. It's quite simple, but I'm wondering if there's something I might have missed. I'm obviously open to feedback on all aspects, because why not, but I'm primarily interested in feedback on correctness and bugs.

/**
 * An implementation of a lock with 3 locking levels. 
 * 
 * The first level is READ - any number of readers may read concurrently as per
 *  the specification of {@link ReentrantReadWriteLock}.
 * 
 * The second level is WRITE - only one writer may write at a time, as per the
 *  specification of {@link ReentrantReadWriteLock}.
 * 
 * The final level is BARGE - a barge request will skip the line to perform an
 * operation with write level access with priority over the others. Competing
 * barge requests will resolve themselves as per the Write specification of
 * {@link ReentrantReadWriteLock} (but they only compete with other Barge
 * requests, not Write requests.)
 * 
 * The intended use case is for mission critical work. Consider users making
 * requests with read and write privileges and a server that needs periodic
 * maintenance routines. The server should not wait in line behind users.
 * Certain admin requests may also need to barge to preempt a malicious actor
 * from flooding the server. If the admin request had to wait in line behind
 * all the malicious requests, untold damage could be done in the meanwhile.
 * 
 */
public class BargeLock {

    /**
     * The typical read-write lock. This implentation should, in time, proxy
     * many of the methods to this.
     */
    private final ReentrantReadWriteLock standard = new ReentrantReadWriteLock(true);

    /**
     * The barge lock - readers and writers of the standard lock will READ from
     * this lock so they all read concurrently. Bargers will write to this, so
     * all other users wait behind the barge.
     */
    private final ReentrantReadWriteLock barge = new ReentrantReadWriteLock(true);

    /**
     * Perform some operation within the safety of a read block, concurrently
     * with other read operations, and blocked by all write and barge operations.
     * @param lambda The operation to perform. Users are responsible for handling
     * input and output outside of this method
     */
    public void performInsideRead(Runnable lambda) {
        try {
            barge.readLock().lock();

            try {
                standard.readLock().lock();

                lambda.run();

            } finally {
                standard.readLock().unlock();
            }
        } finally {
            barge.readLock().unlock();
        }
    }

    /**
     * Perform some operation within the safety of a write block, blocking all
     * read operations and potentially being blocked by earlier arriving write
     * operations, and potentially being blocked by barge operations.
     * @param lambda The operation to perform. Users are responsible for
     * handling input and output outside of this method
     */
    public void performInsideWrite(Runnable lambda) {
        try {
            barge.readLock().lock();

            try {
                standard.writeLock().lock();

                lambda.run();

            } finally {
                standard.writeLock().unlock();
            }
        } finally {
            barge.readLock().unlock();
        }
    }

    /**
     * Perform an operation with priority over all writes and reads. Blocks
     * only from earlier arriving barges.
     * @param lambda The operation to perform. Users are responsible for
     * handling input and output outside of this method.
     */
    public void performInsideBarge(Runnable lambda) {
        try {
            barge.writeLock().lock();

            try {
                standard.writeLock().lock();

                lambda.run();

            } finally {
                standard.writeLock().unlock();
            }

        } finally {
            barge.writeLock().unlock();
        }
    }

}
\$\endgroup\$
4
\$\begingroup\$

There's not much to say, it's not much code, it's documented and from reading it I could not find an error in the locking mechanism.


public void performInsideRead(Runnable lambda) {

A better name would be performSafeRead or performRead, as "inside" does not necessarily give any additional meaning.

lambda should be called action or readAction, as it is not necessarily a lambda.

Having said that, BargeLock might also not be the best name, as it less of a lock, and more of a safe executor.


Given that there is not much else to review, let's talk API design.

public class BargeLock {
    private final ReentrantReadWriteLock standard;
    private final ReentrantReadWriteLock barge;
    
    public BargeLock();
    public void performInsideRead(Runnable lambda);
    public void performInsideWrite(Runnable lambda);
    public void performInsideBarge(Runnable lambda);
}

From an API consumer point of view, it is clear how to use it. As said before, naming could be better, something like this maybe:

public class BargeExecutor {
    private final ReentrantReadWriteLock standardLock;
    private final ReentrantReadWriteLock bargeLock;
    
    public BargeExecutor();
    public void runRead(Runnable readAction);
    public void runWrite(Runnable writeAction);
    public void runBarge(Runnable bargeAction);
}

But if somebody wants to extend that class, it's pretty much impossible. The class is not marked final, so an extension is possible (and maybe wanted in some scenario). But given that only the public methods are accessible, and extension can't do anything useful. You could now make the class final, that would clear that up. However, I hate final classes and methods with a passion, because too often I found myself needing to extend or override that method without alternatives (I'm not talking about Java KeyStore or something, but simple library classes).

So, instead of making it final, it would be interesting if you'd expose the internal state in a half-way safe manner. That can be done by either making standard and barge protected, or by exposing them through protected getters, like this:

public class BargeLock {
    protected final ReentrantReadWriteLock standard;
    protected final ReentrantReadWriteLock barge;
    
    public BargeLock();
    public void performInsideRead(Runnable lambda);
    public void performInsideWrite(Runnable lambda);
    public void performInsideBarge(Runnable lambda);
}
public class BargeLock {
    private final ReentrantReadWriteLock standard;
    private final ReentrantReadWriteLock barge;
    
    public BargeLock();
    public void performInsideRead(Runnable lambda);
    public void performInsideWrite(Runnable lambda);
    public void performInsideBarge(Runnable lambda);
    
    protected final ReentrantReadWriteLock getStandard();
    protected final ReentrantReadWriteLock getBarge();
}

The getters are final because internally the value of the member is being used, not the getter. So overriding the getter in an extending class would yield the false impression that one can change the internal logic of the "perform" functions.

Additionally, the documentation should state clearly in what order the locks are acquired, to allow overriding classes to have the same order to avoid deadlocks.

On another note, currently the executed actions can't throw anything except RuntimeException. You might want to have a custom interface which allows to throw any Exception or Throwable and make it clear that this can happen in your documentation. That would allow a cleaner handling of exceptions in the resulting code:

// Business logic
try {
    barge.performInsideWrite(() -> {
        throw new BusinessException("test");
    });
} catch (BusinessException e) {
    // Handle exception here, assume it's expensive for some reason.
}

Also, if processing exceptions is expensive for some reason, it would reduce lock time.

Returning values from the inside might also be of interest, currently that is not that easily possible.

public <TYPE> TYPE performInsideWrite(Action<TYPE> action) {
    // ...
    return action.run();
    // ...
}

BusinessObject businessObject = barge.performInsideWrite(() -> {
    return new BusinessObject();
});

So, there's that.

\$\endgroup\$
2
  • \$\begingroup\$ The note about exceptions is really good - after thinking about it I think I'm going to create a result object, which will be good for a couple reasons - first, I can make the lock generic so it can be type safe to return the result value, but the result object might also have a throwable that was caught. - so I'll change the lambda from a Runnable to a Producer. While I'm weary of exposing the locks outside the class, I do want to leave it open to extension, though, so making them protected is an improvement. You are correct about the name - it isn't any more a lock than a door is. \$\endgroup\$
    – corsiKa
    Dec 27 '20 at 17:36
  • \$\begingroup\$ I appreciate the thorough review - I'm pressed for time right now (thanks to the holiday, my weekend, well, isn't... hahaha) but I do intend to consider most of these suggestions, and implement many of them. \$\endgroup\$
    – corsiKa
    Dec 27 '20 at 17:38

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.