Review of solution for Cancelled Tasks

[Richard Bair]

> Ok now, a little sleep does wonders ;-)
> 
> Semantic-wise I think we do want something different than in FutureTask:
> basically we want to enable for a user to click on 'cancel' or similar
> which stops a long running operation. Those are our options so far:
> 
> - A cancelled() callback method. Either we call it on the JFX (EDT)
> thread which would seem to be correct, but would potentially open a can
> of worms with synchronization/race conditions between this callback and
> the call() method. Or we call it on the background thread (how?) but
> that would seem unnatural and would actually be difficult (impossible)
> to implement. Worst of all, it does not necessarily stop the background
> task.

That's a good point, it really has to be called on the FX thread (which is right anyway I think). But this method does now exist in 2.1, along with other event handlers and methods for state transitions. That was done for other reasons than for this bug, but can have some usefulness for addressing this particular issue as well, if your needs are simple.

> - Throwing a TaskCancelledException in the background thread. We can
> only do that when the background thread is in one of the updateXXX()
> methods. If it's blocked in any other operation, we cannot do it (we
> cannot throw exceptions asynchronously). Moreover, we might want to do
> one last update on cancel.
> - Using thread interruption. This appears to be the only reasonable and
> safe way to implement it.
> 
> My vote would be for cancel() to call FutureTask.cancel(true)

In fact, that is what I do already:

    @Override public boolean cancel(boolean mayInterruptIfRunning) {
        // Delegate to the super implementation to actually attempt to cancel this thing
        boolean flag = super.cancel(mayInterruptIfRunning);
        // If cancel succeeded (according to the semantics of the Future cancel method),
        // then we need to make sure the State flag is set appropriately
        if (flag) {
            // If this method was called on the FX application thread, then we can
            // just update the state directly and this will make sure that after
            // the cancel method was called, the state will be set correctly
            // (otherwise it would be indeterminate. However if the cancel method was
            // called off the FX app thread, then we must use runLater, and the
            // state flag will not be readable immediately after this call. However,
            // that would be the case anyway since these properties are not thread-safe.
            if (isFxApplicationThread()) {
                setState(State.CANCELLED);
            } else {
                runLater(new Runnable() {
                    @Override public void run() {
                        setState(State.CANCELLED);
                    }
                });
            }
        }
        // return the flag
        return flag;
    }

> , document
> that the call() method should be implemented in a way that it handles
> thread interruption if the developer wants the task to be cancellable.
> For IO streams this involves using NIO InteruptibleChannels. I don't
> think we should throw IllegalStateException or TaskCancelledException
> anywhere. And the cancelled() callback seems to be difficult to
> implement without provocing race conditions.

OK, so Roman votes for #2 -- just document. (#1 was throw the exception, #2 is just document to check for cancellation)

I should also add, that in *any* case all documentation examples will show the use of isCancelled. If there are no strong opinions to throw the exception, then I'm inclined to take Roman's approach. Personally I think I would throw the exception, but I'm so on the fence on this one.

Richard