Refine Locking

[christophstrobl]

Closes: #4429

[jxblum]

The "read" lock should be released inside a finally { .. } block here, particularly since there are interactions with a Logger occurring just above it.

It seems rather benign, but if the LOGGER was not properly initialized, or if the property reference were null when trace is enabled, or some other issues occurred during logging and threw an Exception, then the "read" lock would not be properly released, preventing any writes from occurring, thereby leading to possible Thread starvation.

[mp911de]

synchronized blocks within this class serve the purpose to ensure ordering according (happens before as of the JLS spec). We should keep these to avoid visibility issues of the state field.

[jxblum]

Using Locks has the same visibility guarantees as using a synchronized block. There are no issues so long as 1) the state variable is guarded by the same Lock every time the state variable is accessed and 2) the same Lock is consistently used everywhere the state variable is accessed.

See the Javadoc for Lock, and specifically the section on "Memory Synchronization" (concerns "visibility").

[jxblum]

However, I might add that if CursorReadingTask is being accessed by multiple, concurrent Threads, as is implied by the use of synchronization, now locking, that perhaps a ReentrantReadWriteLock might be a better choice for accessing the state variable (Read Lock on query, and Write Lock on mutation).

[jxblum]

This is the 3rd time this sequence of statements has appeared now. Also visible on lines 91-93, 100-102, and then here (153-155).

Perhaps consider adding a setState(:State) method:

private void setState(State newState) {

    lock.lock();

    try {
        this.state = newState;
    }
    finally {
        lock.unlock();
    }
}

And, I'd still stick to the[ <lock>, <try to do something while lock is held>, <unlock in finally block> ] pattern and idiom, for consistency sake.

[jxblum]

Alternatively, you could introduce an abstract Lock utility class, for other similar lock, action, unlock scoped blocks, as are present throughout the code above (and below), for both LazyLoadingProxyFactory and the CursorReadingTask.

This utility would work for read and write Locks returned by ReadWriteLock, as well.

For instance:

abstract class LockUtils {

    static void doWithLock(Lock lock, Runnable doInsideLock) {

        lock.lock();

        try {
            doInsideLock.run();
        }
        finally {
            lock.unlock();
        }
    }

    static <T> T doWithLock(Lock lock, Supplier<T> doInsideLock) {

        lock.lock();

        try {
            return doInsideLock.get();
        }
        finally {
            lock.unlock();
        }
    }
}

Then, sequences such as:

lock.lock();
this.state = State.CANCELLED;
lock.unlock();

Could be simply replaced with:

LockUtils.doWithLock(this.lock, ()-> this.state = State.CANCELLED);

This could be used in other places here as well, not just the lines noted above.

This would even work for lines, 119-123, enforcing the pattern/idiom to which I refer, along with lines 129-144.

Food for thought.

[jxblum]

For instance, the LockUtils could be used here:

LockUtils.doWithLock(lock, () -> {
    if (State.RUNNING.equals(state) || State.STARTING.equals(state)) {
        this.state = State.CANCELLED;
        if (cursor != null) {
            cursor.close();
        }
    }
});

[jxblum]

With the overloaded Supplier variant, using LockUtils:

public State getState() {
   return LockUtils.doWithLock(lock, () -> this.state);
}

[jxblum]

Example use of LockUtils here:

return LockUtils.doWithLock(lifecycleMonitor.writeLock(), () -> running);

[jxblum]

Using Locks has the same visibility guarantees as using a synchronized block. There are no issues so long as 1) the state variable is guarded by the same Lock every time the state variable is accessed and 2) the same Lock is consistently used everywhere the state variable is accessed.

See the Javadoc for Lock, and specifically the section on "Memory Synchronization" (concerns "visibility").

[jxblum]

However, I might add that if CursorReadingTask is being accessed by multiple, concurrent Threads, as is implied by the use of synchronization, now locking, that perhaps a ReentrantReadWriteLock might be a better choice for accessing the state variable (Read Lock on query, and Write Lock on mutation).

[jxblum]

Should be subscriptionMonitor.readLock().lock();

[christophstrobl]

rebased & updated. I would not want to introduce more public API but moved some code into methods with limited scope.