Review of solution for Cancelled Tasks

[Richard Bair]

The InterruptedException is only thrown if somebody has a blocking call within the "call" method using streams or Thread.sleep or whatnot. Otherwise you get no notification in the call method itself but have to manually ask "is this cancelled?".

If the update methods throw an exception (maybe even InterruptedException...?) then it provides a way for most tasks to know that the thing has been cancelled. But tasks that never update progress or the message or whatnot would still run free if they aren't checking cancelled. But there's nothing we can do there.

I tend to agree with Jim that a checked exception would have been appropriate here. It forces people (annoys them too) to fess up to the fact that this thing might have been cancelled. However that will break people even more at this point (source level breakage).

Another benefit to having the updateXXX methods throw an exception when called from a background thread of a cancelled task is when you write non-looping background code:

doSomething();
updateProgress(1, 4);
doSomething2();
updateProgress(2, 4);
doSomething3();
updateProgress(3, 4);
doSomething4();
updateProgress(4, 4);


In such a case, having to check isCancelled after each doSomething before calling updateProgress is kind of a pain. Instead having an exception lets my code remain simple and easy to read. Ideally we would have a checked TaskCancelledException so that:

try {
    doSomething();
    updateProgress(1, 4);
    doSomething2();
    updateProgress(2, 4);
    doSomething3();
    updateProgress(3, 4);
    doSomething4();
    updateProgress(4, 4);
} catch (TaskCancelledException ex) {
    // do stuff to clean up
    // ...

    // Update the progress and message
    Platform.runLater(() -> {
        updateProgress(4, 4);
        updateMessage("Cancelled");
    });
}

However, I think it is too late to add a checked exception. But we could add the unchecked exception. It will still break some people, but probably not many? I worry about breaking anybody. However if we threw an unchecked exception I could still write the above code if I care about responding to the cancellation in the task, and if I don't, I just let the exception kill the thread and I'm happy as a clam. And if instead of a TaskCancelledException I just threw InterruptedException, then I have a single exception I catch as an indication of cancellation instead of potentially two exceptions.

SO I think the decision has to be:

	1) Throw an unchecked exception from updateXXX whenever called from background thread that has been cancelled (preference might be for InterruptedException)

	or 

	2) Simply document that you have to check for isCancelled and leave it to the developer

Option #1 will perhaps lead to cleaner code, but it will break some people and will introduce a little bit of semantic complexity. Option #2 will result in more isCancelled checks in Task implementations, but breaks nobody.

I have both options implemented, so it is just a matter of choosing :-). I'm torn. We really can't do #2 now and #1 later, because the documentation is going to say that you can call updateXXX after the thread is cancelled, so it really has to be a choice made now.

Vote for favorite? #1 or #2? After the general approach is decided, specifics can be hashed. If this were before 2.0 I would prefer #1, right now I'm leery about breaking folks.

Richard

On Jan 4, 2012, at 10:21 PM, Tom Eugelink wrote:

> 
> I suggested that too (throwing a checked exception name TaskCancelledException), but would required an API change. I was wondering; where did the InterruptedException go in Task? It's the one thing that should be handled AFAIK. And an exception is the only clean way for a thread to clean up its monitors and stuff.
> 
> The essence of this problem is a Java one, not a JavaFX one. Even though I like all the ideas, they still seem like a band-aid to me. OTOH, exceptions being thrown is not such a exceptional situation. Suppose the ISE is thrown after the progress values are set. One could get things like:
> 
> try
> {
>    if (isCancelled())
>    {
>        setProgress(100%) // this throws exception, normally this would be a break or return
>    }
>    setProgress(counter)
> }
> finally
> {
>    cleanup()
> }
> 
> 
> Or maybe a specialized method that does not throw the ISE xception
>    setProgress(100%, false)
>    setProgressNoISE(100%)
>    setProgressToFinished()
> 
> Tom
> 
> 
> 
> On 2012-01-05 02:26, Jim Graham wrote:
>> This was the original reason behind checked exceptions.  IO operations were the main target because they are the thing that most threads end up in when they really should have been cancelled - and also because those methods are blocking and we needed an exceptional return value.
>> 
>> Simply declaring updateProgress() and some other key methods to declare that they throw Interrupted will mean that the developer will be forced to opt in by virtue of having to catch the exception.
>> 
>> It's worked reasonably well for Java I/O for a while, though I'll admit it is an annoyance for programmers - but it is a healthy annoyance...
>> 
>>            ...jim
>> 
>> On 1/4/2012 11:39 AM, Jeff Martin wrote:
>>> Another idea might be to throw the InterruptedException from a method called fireInterruptedException(), so savy rollback Tasks could override it to do nothing.
>>> 
>>> jeff
>>> 
>>> 
>>> On Jan 4, 2012, at 1:24 PM, steve.x.northover at oracle.com wrote:
>>> 
>>>> One (wild/bogus/too late?) idea might be to combine checking for cancel with progress update.  That way, programmer can't do one without the other.
>>>> 
>>>> Steve
>>>> 
>>>> On 04/01/2012 2:11 PM, Richard Bair wrote:
>>>>> Ya, that was exactly what I was proposing :-). If the updateXXX methods throw an ISE when called from a background thread of a cancelled task, then you will generally get the behavior you instinctively want -- the thread dies (most of the time) and the thing stops getting updated.
>>>>> 
>>>>> However this means that when you actually DO want to update the progress, message, or title when the thing is cancelled, then you need to do it in the cancelled() method (which is called on the FX app thread) or you have to use a Platform.runLater() if you handle it from the call() method.
>>>>> 
>>>>> So throwing the exceptions will cause some exceptions in existing apps that weren't happening before (although maybe that is good since it will, if left unhandled, kill the thread). It will make it more difficult for people who are wanting to update the title / message / progress after the thing is cancelled.
>>>>> 
>>>>> In either case all use cases are possible, it is just a matter of which you bias for. Do you bias for the naive implementation or the sophisticated implementation? Generally I always bias for the naive implementation because even great developers are happier when things "just work" the way their intuition dictates and the exceptions give enough context that developers who want to do the advanced stuff can determine how it is done.
>>>>> 
>>>>> Richard
>>>>> 
>>>>> 
>>>>> On Jan 4, 2012, at 11:05 AM, steve.x.northover at oracle.com wrote:
>>>>> 
>>>>>> Hmmm .. perhaps they should throw an exception, not update the progress and not cancel nicely?  I suppose that this "cancels" the task!
>>>>>> 
>>>>>> Steve
>>> 
>> 
> 
>