Review of solution for Cancelled Tasks

[Richard Bair]

Hi Roman!

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

Right. updateXXX is not blocking, and I hadn't thought of checking whether InterruptedException is checked.

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

It is trivial. In the updateProgress method, I just do two checks: isCancelled() and a thread check. If the thread is not the FX App thread and isCancelled(), then throw an exception. In the above code I'm using Platform.runLater to run the updateProgress and updateMessage on the FX app thread, where it would still be legal.

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

Ya, I like this too and would do it this way in most cases. However what about the case where you want one message on cancel if it happens during the first part of your task, but another message later in the task? Like "Cancelled while connecting" vs. "Cancelled while downloading data". In such a case you would need to have some thread safe state to communicate what you are doing so that from the cancelled method you can post the right message (note that cancelled() is called on the FX App thread, which is why it is safe to call updateMessage from there).

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

I see, so in this situation you would have cancelled be called on the background thread as opposed to the fx thread. I think I like it better when called on the FX thread so that only a single method, "call" is invoked on a background thread all others on the FX thread. This way developers know that only this one method will be called by the system on a background thread.

Cheers
Richard