In NettyResponseFuture, abort and done methods are not mutually exclusive

[PierreCoquentin]

Hi,

We are using async-http-client 1.7.10 with netty as provider.
In NettyResponseFuture, abort and done methods are not mutually exclusive. If the reaper wakes up at the same time we received the response, then the asyncHandler onThrowable method (line 314) and onCompleted method (line 262) are called. Indeed, the AtomicBoolean "done" is only set after the completion of getContent method. More precisely, if the reaper thread preempts the worker thread while executing the handler method onCompleted, it seems that nothing prevents the method onThrowable of the handler to be called by the reaper.

   V getContent() throws ExecutionException {
        ExecutionException e = exEx.getAndSet(null);
        if (e != null) {
            throw e;
        }

        V update = content.get();
        // No more retry
        currentRetry.set(maxRetry);
        if (exEx.get() == null && !contentProcessed.getAndSet(true)) {
            try {
// Line 262 : The worker is preempted by the reaper thread here (The exception has not been set)
                update = asyncHandler.onCompleted();
            } catch (Throwable ex) {
                if (!throwableCalled.getAndSet(true)) {
                    try {
                        asyncHandler.onThrowable(ex);
                    } catch (Throwable t) {
                        logger.debug("asyncHandler.onThrowable", t);
                    } finally {
                        cancelReaper();
                        throw new RuntimeException(ex);
                    }
                }
            }
            content.compareAndSet(null, update);
        }
        return update;
    }

    public final void done(Callable callable) {
        try {
            cancelReaper();

            if (exEx.get() != null) {
                return;
            }
            getContent();
            isDone.set(true);
            if (callable != null) {
                try {
                    callable.call();
                } catch (Exception ex) {
                    throw new RuntimeException(ex);
                }
            }
        } catch (ExecutionException t) {
            return;
        } catch (RuntimeException t) {
            exEx.compareAndSet(null, new ExecutionException(t));
        } finally {
            latch.countDown();
        }
        super.done();
    }

    public final void abort(final Throwable t) {
        cancelReaper();

// The done boolean has not been set by the worker thread
        if (isDone.get() || isCancelled.get()) return;
// It's too late to set the exception as the worker thread has already checked
        exEx.compareAndSet(null, new ExecutionException(t));
        if (!throwableCalled.getAndSet(true)) {
            try {
// (line 314) Finally the reaper thread executes the onThrowable method
                asyncHandler.onThrowable(t);
            } catch (Throwable te) {
                logger.debug("asyncHandler.onThrowable", te);
            } finally {
                isCancelled.set(true);
            }
        }
        latch.countDown();
        super.done();
    }

Regards
Pierre

[slandelle]

Both issues you opened seem to point out that there's some clean up to be done in NettyResponseFuture.
Let's do that for AHC 2.

[slandelle]

Sorry for taking so long. Thanks!