AbstractListenableFuture#addListener does not execute listener when the listener is added after the future completion and when the execution list is not yet initialized

[nithril]

You could find below two test cases to add to org.asynchttpclient.ListenableFutureTest:

• ListenableFutureTest#testListenableFutureAfterCompletion with the issue when the listener is added after the completion
• ListenableFutureTest#testListenableFutureBeforeAndAfterCompletion without the issue because the execution list is initialized prior the future completion

    @Test(groups = "standalone")
    public void testListenableFutureAfterCompletion() throws Exception {

        AtomicInteger counter = new AtomicInteger(1);

        try (AsyncHttpClient ahc = asyncHttpClient()) {
            final ListenableFuture<Response> future = ahc.prepareGet(getTargetUrl()).execute();
            Response response = future.get();
            future.addListener(() -> counter.decrementAndGet(), Runnable::run);
        }
        Assert.assertEquals(0 , counter.get());
    }


    @Test(groups = "standalone")
    public void testListenableFutureBeforeAndAfterCompletion() throws Exception {

        AtomicInteger counter = new AtomicInteger(2);

        try (AsyncHttpClient ahc = asyncHttpClient()) {
            final ListenableFuture<Response> future = ahc.prepareGet(getTargetUrl()).execute();

            future.addListener(() -> counter.decrementAndGet(), Runnable::run);

            Response response = future.get();
            future.addListener(() -> counter.decrementAndGet(), Runnable::run);
        }
        Assert.assertEquals(0 , counter.get());
    }

[nithril]

For instance, I do not see a fix that does not imply to add an executed boolean field to AbstractListenableFuture. It will duplicate a part of the ExecutionList code

[slandelle]

Yep. It seems I broker it when I tried to lazy load the ExecutionList.
Working on a fix.

Thanks for reporting.

[nithril]

Thanks for this fix.

If I understand correctly the fix, just before the completion, more than one threads can execute concurrently the methods executionList().run() and thus the thread executor/context may be different.

1. The thread that complete the future will set the hasRun flag and calls runListeners
2. All threads that call addListener before 1. runListeners complete will process the runnables list too

Does it look like an issue for you?

[slandelle]

Runnables will be processed only once, as they're stored in a LinkedBlockingQueue.
But you have no guarantee about the running thread and the ordering when you register listeners after completion because calling thread might be "stealing" work from the IO thread.

[nithril]

I agree with your first statement. My comment is about your second statement, maybe updating the javadoc could help to understand this side effect?

I wonder if AbstractListenableFuture#hasRun can now supersede ExecutionList#executed. And thus if the ExecutionList synchronized blocks can be removed ?

Another way: the ExecutionList#add method can return a boolean to the caller to mark the Runnable as executed and avoid a runListeners.

Whatever thanks for this fix. 👍

[slandelle]

I wonder if AbstractListenableFuture#hasRun can now supersede ExecutionList#executed. And thus if the ExecutionList synchronized blocks can be removed ?

Not sure. And I'd rather avoid changing things here as this class is a fork from Guava.