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?".
> 
> Interestingly, I/O streams are one of the things that are not
> interruptible (i.e. don't throw InterruptedException, and when you call
> Thread.interrupt() on a thread that's in an IO, it won't do anything).
> You can get interuptible IO only by using NIO. You can still check
> Thread.isInterrupted() though (but only after the IO operation
> unblocks).

Huh, I don't know where I picked that up from. I just googled though and found:

http://docs.oracle.com/javase/1.4.2/docs/api/java/io/InterruptedIOException.html

So it looks like an InterruptedIOException is thrown, not an InterruptedException. Drat, another potential exception type that gets thrown when the thing is 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
> 
> Another option would be to call Thread.interrupt() to signal that the
> thread has been interrupted. Any call to a method that is blocking would
> throw an InterruptedException then. App code can check
> Thread.isInterrupted() if it wants to bail out even if it doesn't call
> blocking code. I am not even sure that we need a notion of 'isCancelled'
> if the interrupted flag does exactly this. On the other hand, we might
> want to use the interrupted flag internally, and - for example - throw
> an unchecked TaskInterruptedException from JavaFX code, as well as
> blowing up in blocking calls with InterruptedException.
> 
> What do you think?

I actually tried to unit test that and it didn't work. The implementation of cancel is handled by the concurrency libraries (FutureTask.cancel to be precise). I checked the thread interrupt status but it said it was not interrupted even after a cancel. But in any case, isCancelled is already there, it is part of the FutureTask API.

Cheers
Richard