Actions not executed for cancelled futures

[ummels]

According to the documentation, actions registered with onFailure should be executed when a future is cancelled. However, this does not seem to be the case as the following code prints nothing:

Future<?> f = Future.of(() -> { while (true) {} });
f.onFailure(e -> println("Failure"));
f.cancel();
if (!f.isFailure())
    println("Oops");
Thread.sleep(1000L);
System.exit(0);

[nfekete]

Note that while this doesn't affect the bug itself, while (true) {} is not interruptible, i.e., it will never release the thread back to the ExecutorService, even if you call cancel().

[ummels]

Thanks for the info. I guess while (true) { Thread.sleep(100L); } would be a better example.

[ruslansennov]

It seems that the problem code is FutureImpl.java#L199. Computation fails in Try block with InterruptedException which is fatal according to Try.java#L1493.

I believe we should use classic try {} catch () {} block here.
See #1976

[ruslansennov]

And maybe actions.forEach(this::perform) should be here

[ruslansennov]

see #1963

[danieldietrich]

I think the FutureImpl.cancel(boolean) function needs to look like this:

    @Override
    public boolean cancel(boolean mayInterruptIfRunning) {
        synchronized (lock) {
            if (isCompleted()) {
                return false;
            } else {
                return Try.of(() -> job == null || job.cancel(mayInterruptIfRunning)).onSuccess(cancelled -> {
                    if (cancelled) {
                        complete(Try.failure(new CancellationException()));
                    }
                }).getOrElse(false);
            }
        }
    }

Interestingly we already have a unit test:

    @SuppressWarnings("InfiniteLoopStatement")
    @Test
    public void shouldInterruptLockedFuture() {
        final Future<?> future = Future.of(() -> {
            while (true) {
                Try.run(() -> Thread.sleep(100));
            }
        });
        future.onComplete(r -> fail("future should lock forever"));
        future.cancel();
        assertCancelled(future);
    }

After the fix is applied, the onComplete handler, which throws an AssertionError, is executed. Currently the onComplete actions are executed on different threads (which will be changed with #1530), so the unit test is still green.