Review of solution for Cancelled Tasks

[Roman Kennke]

Hi Richard,

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

Uhm but it requires a specific pattern which is not obvious: use
runLater() in the catch handler and not use runLater() in the try block.
Good luck explaining that to developers! ;-)

> > 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 same can be said in the exception-throwing case: you need to keep
some state somewhere. Ok it doesn't need to be threadsafe and can infact
be a local variable instead of a field, but still.

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

I am a little undecided. I now come to think that since Task.cancel() is
implemented by FutureTask.cancel() it should carry the same semantics.
I.e. indicate that a task should not start running if it isn't started
yet, and *maybe* interrupt a running task/thread (via a boolean
parameter) in which case the implementer of the call() method must
implement it in such a way that it handles interruption
correctly/gracefully (e.g. check for Thread.isInterrupted() in a loop or
handle InterruptedException when it does Thread.sleep(), etc). Which
basically boils down to simply documenting all this. Hfrmp... I guess I
need to go inside me a little more (it's too late around here..). 

Cheers, Roman