Review of solution for Cancelled Tasks

[Roman Kennke]

Hi Richard,

> >> 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 :-(

In my experience, when you call Thread.interrupt() on a thread that is
blocked in a network IO, it will not throw this exception. The only way
to 'cancel' such IO is to close() the stream, which will result in an
IOException in the blocked thread. I am not sure what this exception is
actually used for. Some stuff I found in the web seems to indicate it's
only used for PipedStreams:

http://docstore.mik.ua/orelly/java/fclass/ch11_30.htm

In order to be sure, we probably need to consult an IO expert or check
the java.io code ourselves, but I am pretty sure that at least network
streams are not interruptible with java.io streams (might be different
for files or pipes or whatnot). Which was the whole point why
InterruptibleChannels have been introduced in NIO.

> > 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.

Interesting. Well I guess the semantics is somewhat different:

- Cancel means to signal a task (that might hang out on a queue until it
gets processed) that its execution should not be started. If it's
already started, only cancel(true) *can* stop it (if the task handles
interruption). It does not provide a mechanism to roll up a thread.
- Interruption is a mechanism to cleanly terminate an already running
task. This is completely opt-in, i.e. the task must be implemented to
actually handle thread interruption, e.g. when you have a long-running
loop that calculates stuff (i.e. it is blocking), and you want to make
it interruptible, you need to check Thread.isInterrupted() inside the
loop and either throw InterruptedException or handle the interruption
correctly otherwise. It's a bit complicated to get right, I always refer
to this excellent article:

http://www.ibm.com/developerworks/java/library/j-jtp05236/index.html

I guess the question to ask in our case is:

1. is updateXXX() blocking? No it is not, as far as I can see (correct
me if I'm wrong). Therefore it should not throw InterruptedException or
handle interruption in any way. Further, because InterruptedException is
checked, and you already pointed out that we can't add a checked
exception, this is not an option anyway.
2. How do we want to handle cancellation then:

>> 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");
>>    });
>> }

I think in general this is fine. But implementation-wise, how do you
make updateProgress() throw TaskCancelledException in the try block,
while not doing the same in the catch block?

I like this approach (that you sent in your first email):

new Task() {
    public Object call() throws Exception {
        for (int i=0; i<100; i++) {
            if (isCancelled()) break;
            try {
                doSomething();
                updateProgress(i, 100);
             } catch (Exception e) { }
        }
        return null;
    }

    @Override protected void cancelled() {
        updateMessage("Cancelled");
    }
};

The cancelled() method would be called as soon as the task gets
cancelled, where the application can perform the necessary cleanup, call
updateProgress() or whatever without blowing up.

Maybe this should be combined with throwing the TaskCancelledException
(but only *after* the above call to cancelled() returned) to signal that
the task can stop doing whatever it is doing, and roll up the executing
thread.

What do you think?

Cheers, Roman