Review of solution for Cancelled Tasks

[steve.x.northover at oracle.com]

+1 for not fixing faulty code.  People who write background task code 
need to play by the rules and check for their task being canceled.

Steve

On 04/01/2012 8:11 AM, Roman Kennke wrote:
> Hi Richard,
>
> I agree with the other's sentiments that JavaFX should not try to fix
> faulty code.
>
> I would lean to do more or less what is done in java.util.concurrent,
> especially the FutureTask class. I.e. provide an API to check
> isCancelled() and otherwise use InterruptionException to correctly
> interrupt a thread:
>
> http://www.ibm.com/developerworks/java/library/j-jtp05236/index.html
>
> In my experience, APIs that let people write shitty code leads to a lot
> of shitty code being written (surprise!), especially when threads are
> involved.
>
> Cheers, Roman
>
> Am Dienstag, den 03.01.2012, 20:05 -0800 schrieb Richard Bair:
>> http://javafx-jira.kenai.com/browse/RT-17932
>>
>> I have a proposed solution for this bug, if anybody has a few minutes to give it some thought that would be great. In a nutshell:
>>
>> 	- When a task is cancelled, the background thread might continue to operate. This is the way Java works (we can't safely kill a thread outright, so we have to wait for the thread to terminate)
>> 		- The developer of a Task can check isCancelled from the background thread and self-terminate
>> 		- The developer can use any API that throws an InterruptedException and use that to indicate that the thread has been cancelled
>> 	- A "run-away" background thread that is supposed to be cancelled, can continue to update the text, message, and progress of a task, giving the appearance that it isn't cancelled!
>> 	- A cancelled task might want to legitimately update the text, message, or progress in the case of cancellation (maybe the message becomes "I'm cancelling this request").
>>
>> In trying to solve this problem, I wanted a solution where the naive implementation still works as one would expect, and a well considered solution works perfectly. I think I have a solution.
>>
>> If you write a Task, and do nothing smart:
>>
>> new Task() {
>>      public Object call() throws Exception {
>>          for (int i=0; i<100; i++) {
>>              doSomething();
>>          }
>>          return null;
>>      }
>> };
>>
>> In this case, if the task is cancelled, then it will just keep running in the background, but since the task returns nothing and never updates the progress or text or message, it appears to have been successfully cancelled to any code using the task. Here is another dumb implementation:
>>
>> new Task() {
>>      public Object call() throws Exception {
>>          for (int i=0; i<100; i++) {
>>              doSomething();
>>              updateProgress(i, 100);
>>          }
>>          return null;
>>      }
>> };
>>
>> This one will update the progress, and exhibits the bug. My proposal is that calling updateProgress, updateMessage, or updateText from a background thread of a cancelled Task will result in an IllegalStateException. So in this case, the attempt to updateProgress after the task was cancelled will actually cause the exception and, in this case, actually terminate the background thread. Two birds, one stone.
>>
>> In this case...:
>>
>> new Task() {
>>      public Object call() throws Exception {
>>          for (int i=0; i<100; i++) {
>>              try {
>>                  doSomething();
>>                  updateProgress(i, 100);
>>               } catch (Exception e) { }
>>          }
>>          return null;
>>      }
>> };
>>
>> ...we are not so lucky. Although the progress won't be updated on a cancelled task, it will keep running in the background. But no harm, no foul. If the developer was smart they'd do something like:
>>
>> 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;
>>      }
>> };
>>
>> Anyway, it seems that this solution doesn't hurt the naive case, and in some instances helps it, and in any case for more sophisticated library developers you have the tools to handle cancellation correctly. Here are two ways to correctly update the text after the task has been cancelled:
>>
>> new Task() {
>>      public Object call() throws Exception {
>>          for (int i=0; i<100; i++) {
>>              if (isCancelled()) {
>>                  Platform.runLater(new Runnable() {
>>                      public void run() {
>>                          updateMessage("Cancelled")
>>                      }
>>                  });
>>                  break;
>>              }
>>              try {
>>                  doSomething();
>>                  updateProgress(i, 100);
>>               } catch (Exception e) { }
>>          }
>>          return null;
>>      }
>> };
>>
>> 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");
>>      }
>> };
>>
>> I think this works fairly well. Anything I'm missing?
>>
>> Thanks!
>> Richard
>