Issues with Callbacks about tascalate-concurrent HOT 11 CLOSED

Chris,

1. I hardly understand the issues mentioned without code that shows it. Please attach some test cases that manifest the problem.
2. Also, for the second issues it sounds like an expected behavior, please explain how it should behave. Also "the first promise will not terminate" is a vague term here: terminate normally/exceptionally/cancelled?

Regards,
Valery

from tascalate-concurrent.

Check this test case for part 1 of the problem:

@Test
    public void test_shutting_down_executor_will_not_hang_promise_due_to_pending_callback() throws ExecutionException, InterruptedException {
        TaskExecutorService singleThreadExecutor = TaskExecutors.newSingleThreadExecutor(new ThreadFactoryBuilder()
                .setDaemon(true)
                .setNameFormat("Bla-%d")
                .build());

        CountDownLatch latch = new CountDownLatch(1);
        AtomicBoolean flag = new AtomicBoolean();
        Promise<Void> p = CompletableTask.asyncOn(executor)
                .thenApplyAsync(v -> {
                    try {
                        latch.await();
                    } catch (InterruptedException ex) {
                    }
                    return v;
                }, singleThreadExecutor)
                .handleAsync((v, t) -> {
                    flag.set(true);
                    return null;
                });

        singleThreadExecutor.shutdown();
        latch.countDown();
        //singleThreadExecutor.awaitTermination(10, TimeUnit.SECONDS);
        p.get();
        assertTrue(flag.get());
    }

Above, p.get() will not terminate due to an rejection exception being thrown inside the promise.
The second part is probably just due to me having used .handle() instead of .handleAsync() with the old executor. I had assumed that specifying an executor in an async method will not overwrite the default executor specified before.

from tascalate-concurrent.

Oh, another note: For the second part, I assumed that CompletableTask.asyncOn(executor) would behave the same as .defaultAsyncOn(executor), but it doesn't.

from tascalate-concurrent.

This is by design:

Promise<Void> p1 = CompletableTask.asyncOn(firstExecutor);
Promise<Void> p2 = p1.thenApplyAsync(..., secondExecutor);

p1 will use firstExecutor for any async operations that do not specify explicit executor;
p2 will use secondExecutor for any async operations that do not specify explicit executor;

Every operation returns a new promise, and it will either inherit defaultExecutor of originating promise (if no explicit executor is specified for operation) or will use explicitly supplied executor as default.

Promise.defaultAsyncOn is designed explicitly to alter this default behavior -- it returns a Promise wrapper with re-bound defaultExecutor.

In net.tascalate.concurrent.AbstractCompletableTask line 427+ :

    private <U> AbstractCompletableTask<U> internalCreateCompletionStage(Executor executor) {
        // Preserve default async executor, or use user-supplied executor as default
        // But don't let SAME_THREAD_EXECUTOR to be a default async executor
        return createCompletionStage(executor == SAME_THREAD_EXECUTOR ? getDefaultExecutor() : executor);
    }

Here is executor param passed to method is what you supply via thenApply(..., executor)

So, again, this is by design and not a bug.

from tascalate-concurrent.

To clarify the logic behind this design... Imaging, you are reading the source code:

CompletableTask
  .asyncOn(rootExec)
  .thenApplyAsync(..., exec1) 
  .thenApply(...) //A
  .handleAsync(...) //B
  .whenCompleteAsync(..., exec2)
  .thenRunAsync(); //C
  .;

How would you guess, what executor is used at points A, B, C? With my logic it's very simple: look at previous steps in the chain (bottom-to-top, starting from the closest) and see the most recently mentioned executor. So in A and B it will be exec1 (and not rootExec !!!) and point C it will be exec2 (rahter than exec1 or rootExec).

I hope you agree that this makes sense. :)

Regards,
Valery

from tascalate-concurrent.

For the second part, I assumed that CompletableTask.asyncOn(executor) would behave the same as .defaultAsyncOn(executor), but it doesn't.

In fact, they both do the same thing: specify what executor will be used implicitly in *Async operations when no explicit executor is passed

from tascalate-concurrent.

The documentation should be clarified, especially what is regarded as a "default" and what calls override these defaults. Nevertheless: Specifying asyncOn() and .defaultAsyncOn() is NOT the same. Have you tried the provided test case? It will hang on p.get() with only asyncOn() as the handleAsync() is attempted to be executed on the shutdown executor (and throw an Exception and THAT is the bug you need to be fixing, regardless of the behavior of asyncOn()/defaultAsyncOn()), while when using

        Promise<Void> p = CompletableTask.asyncOn(executor)
                .defaultAsyncOn(executor)

The test will pass because the .handleAsync() without the explicit executor will revert back to the originally specified default executor in .defaultAsyncOn() that is NOT overwritten by the call to .thenApplyAsync().

from tascalate-concurrent.

The test will pass because the .handleAsync() without the explicit executor will revert back to the originally specified default executor in .defaultAsyncOn() that is NOT overwritten by the call to .thenApplyAsync().

Right. Because ExecutorBoundPromise (used by defaultAsyncOn) propagates own default executor further (wraps results of own thenApply / thenCombine / etc).

Have you tried the provided test case? It will hang on p.get() with only asyncOn() as the handleAsync() is attempted to be executed on the shutdown executor (and throw an Exception and THAT is the bug you need to be fixing, regardless of the behavior of asyncOn()/defaultAsyncOn())

Sure I tried your test. It throws synchronous exception on adding callbacks. Will provide a fix shortly.

from tascalate-concurrent.

Thanks!

from tascalate-concurrent.

Fix committed, please verify.
New version will be released shortly

from tascalate-concurrent.

As a bonus pack -- CompletableTask.asyncOn now has additional parameter enforceDefaultAsync that let you create a chain with enforced default executor.

from tascalate-concurrent.