Review of solution for Cancelled Tasks

[Roman Kennke]

Ok now, a little sleep does wonders ;-)

Semantic-wise I think we do want something different than in FutureTask:
basically we want to enable for a user to click on 'cancel' or similar
which stops a long running operation. Those are our options so far:

- A cancelled() callback method. Either we call it on the JFX (EDT)
thread which would seem to be correct, but would potentially open a can
of worms with synchronization/race conditions between this callback and
the call() method. Or we call it on the background thread (how?) but
that would seem unnatural and would actually be difficult (impossible)
to implement. Worst of all, it does not necessarily stop the background
task.
- Throwing a TaskCancelledException in the background thread. We can
only do that when the background thread is in one of the updateXXX()
methods. If it's blocked in any other operation, we cannot do it (we
cannot throw exceptions asynchronously). Moreover, we might want to do
one last update on cancel.
- Using thread interruption. This appears to be the only reasonable and
safe way to implement it.

My vote would be for cancel() to call FutureTask.cancel(true), document
that the call() method should be implemented in a way that it handles
thread interruption if the developer wants the task to be cancellable.
For IO streams this involves using NIO InteruptibleChannels. I don't
think we should throw IllegalStateException or TaskCancelledException
anywhere. And the cancelled() callback seems to be difficult to
implement without provocing race conditions.

Regards, Roman



Am Donnerstag, den 05.01.2012, 23:45 +0100 schrieb 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
> 
> 
>