`QueryIterPlainWrapper` concurrency on `close`/`cancelRequest`

[afs]

This is from #3753 (comment)

There is a race condition in QueryIterPlainWrapper and QueryIterAbortable which inherits from QueryIterPlainWrapper.

requestClose and close both call closeIterator but from different threads. closeIterator clears the iterator member.

Using synchronized on closeIterator and close should fix observed test failures (#3751).

However, these operations may cause internal state on the wrapped iterator.
hasNextBinding / moveToNextBinding / close (normal operation) will running on the query executiuon thread, so no concurrency issues,

But requestClose is called on a separate thread and it too may manipulate the internal state on the wrapped iterator.

[afs]

One solution is a volatile so requestCancel sets this and it is picked up by the query execution thread at the start of hasNextBinding / moveToNextBinding and then call closeIterator on the query execution thread.

ExecutionContext.checkCancelSignal (and then no need for QueryIterPlainWrapper.requestCancel) might achive this.

If so, then there may be several places where code can be simplified by replacing getting/testing the cancel signal with checkCancelSignal.

[Aklakan]

The thing is, that QueryIterPlainWrapper wraps a plain iterator. Plain iterators may have close() but they don't have abort.
Another solution (same idea but more invasive) could be to add abort to atlas iterators: Move the abort logic from QueryIteratorBase to an atlas AbortableIterator and have both atlas Iterators and the revised QueryIteratorBase inherit the same mechanism.

So what I am trying to say: All iterators involved in query execution should ideally be abortable (as they might either be backed by resources that may block or perform long computations for each item) - and in the few cases where an Iterator really doesn't implement abort (should be avoided), one just sets a volatile abort flag and have hasNextBinding() return false.

[Aklakan]

In summary, the two semantics are:

• QueryIterPlainWrapper with void requestCancel() { }
• QueryIterPlainWrapperCloseOnAbort with void requestCancel() { closeIterator(); } and some synchronization mechanism.

[afs]

All iterators involved in query execution should ideally be abortable

the work of the abort wil be on the query execution thread (which maybe inside a transaction, and the signal is from outside). For QueryIterAbortable it can be part of close.

QueryIterPlainWrapperCloseOnAbort is what QueryIterAbortable is trying to be for clear-up, not interrupt, style abort.

The query execution thread has to throw QueryCancelledException so that it works it way up through the transaction mechanism and to the QueryExec.

It is possible there could be operations that are in middle of something when the cancel signal is set and need to be poked c.f. thread interrupt.

I can't think of any in the ARQ/DBOE/TDB code. Are there any in the service enhancer?

It's a similar situation (not quite as serious as it isn't potentially VM or retaining locks) to Thread.stop.

Using the execution context cancel:

    @Override
    protected boolean hasNextBinding() {
        possibleCancellation();
        return iterator.hasNext();
    }

    private void possibleCancellation() {
        ExecutionContext execCxt = super.getExecContext();
        if ( execCxt == null )
            return;
        AtomicBoolean cancelSignal = execCxt.getCancelSignal();
        if ( cancelSignal != null && cancelSignal.get() ) {
            closeIterator();
            throw new QueryCancelledException();
        }
    }

(AtomicBoolean.get may be stronger than is needed).

[Aklakan]

Are there any in the service enhancer?

The 'old' service enhancer still ignores cancel, the revised one with PR #3706 should cancel properly and not block if the underlying iterators don't block.

private void possibleCancellation()

Isn't this the logic that is effectively already in place in QueryIteratorBase that is a super class of QueryIterPlainWrapper?
If so, then this change in QueryIterPlainWrapper should be sufficient:

     @Override
     protected void requestCancel()
-    { closeIterator(); }
+    { }

And the removal of the synchronized from closeIterator.

Excerpt from QueryIteratorBase (requestingCancel is the AtomicBoolean extracted from the ExecutionContext):

    private boolean requestingCancel() {
        return (requestingCancel != null && requestingCancel.get()) || Thread.currentThread().isInterrupted() ;
    }

      @Override
    public final Binding nextBinding() {
        try {
            // Need to make sure to only read this once per iteration
            boolean shouldCancel = requestingCancel();

            if ( shouldCancel ) {
                // Try to close first to release resources (in case the user
                // doesn't have a close() call in a finally block)
                close();
                throw new QueryCancelledException();
            }
        // ...
        }
    }