FutureCallback onFalure called on cancelling a future

[GoogleCodeExporter]

If creating a future with FutureCallback, there is no way to identify if the 
future was cancelled. On cancellation, a cancellationException is thrown which 
may not have been intended.
Example:

ListeningExecutorService service = 
MoreExecutors.listeningDecorator(Executors.newFixedThreadPool(10));
ListenableFuture<Explosion> explosion = service.submit(new 
Callable<Explosion>() {
  public Explosion call() {
    return pushBigRedButton();
  }
});
Futures.addCallback(explosion, new FutureCallback<Explosion>() {
  // we want this handler to run immediately after we push the big red button!
  public void onSuccess(Explosion explosion) {
    walkAwayFrom(explosion);
  }
  public void onFailure(Throwable thrown) {
    battleArchNemesis(); // escaped the explosion!
  }
});

explosion.cancel(true);

This will result in battlingArchNemesis. 
What if the FutureCallback had another method called onCancel:
Futures.addCallback(explosion, new FutureCallback<Explosion>() {
  // we want this handler to run immediately after we push the big red button!
  public void onSuccess(Explosion explosion) {
    walkAwayFrom(explosion);
  }
   public void onCancel() {
    noNeedToBattleArchNemesis(); //no explosion.
  }
  public void onFailure(Throwable thrown) {
    battleArchNemesis(); // escaped the explosion!
  }
});

Original issue reported on code.google.com by karan.r...@priceline.com on 24 Jul 2014 at 7:48

[GoogleCodeExporter]

In practice, we're stuck with things as they are because FutureCallback is a 
non-@Beta interface. In case it makes you feel better, I'll make a case for why 
things are the way they are:

addCallback/FutureCallback is a shortcut: Anything that it does can be done 
with a plain addListener/Runnable. Consequently, addCallback need not cover all 
possible use cases. In fact, when covering an unusual use case makes a common 
use case more verbose, it may be best not to cover that case. I contend that 
that's the case here: Many Futures are known to never be cancelled, or, if they 
are cancelled, this is as good as failure to their users. Forcing such users to 
implement onCancel() would add code that doesn't benefit them. (An alternative 
would be for FutureCallback to be an abstract class with an empty, overrideable 
onCancel method. Our fear there was that users who need to take action no 
matter how a Future completes might not realize that they need to override it. 
Maybe we could solve this problem with a more sophisticated scheme, one that 
uses onCancel if it has been overridden but onFailure otherwise, but this would 
be unusual behavior in Java.) And anyway, determined users still have a way of 
determining whether a Future was cancelled from a FutureCallback:

  public void onFailure(Throwable thrown) {
    if (explosion.isCancelled()) {
      noNeedToBattleArchNemesis(); //no explosion.
    } else {
      battleArchNemesis(); // escaped the explosion!
    }
  }

Finally, I'll make a further case that, for Futures in general, "if they are 
cancelled, this is as good as failure to their users." When a Future is 
cancelled, its underlying work (in your case, a Callable) may or may not have 
completed. The exact outcome depends on the timing of the cancel() call, the 
boolean argument to cancel(), and the Future implementations involved. For 
example, it may be that pushBigRedButton() was called and the explosion 
occurred but, before pushBigRedButton() returned, the Future was cancelled. In 
this case, our hero had best still walk away from the explosion. To implement 
this, *any* onFailure/onCancel callback would need to consult 
hasExplosionOccurred(). (Or perhaps cancellation of the explosion Future is 
just going to turn out to be dangerous in general.)

Original comment by cpov...@google.com on 25 Jul 2014 at 2:50

• Changed state: WorkingAsIntended

[GoogleCodeExporter]

This issue has been migrated to GitHub.

It can be found at https://github.com/google/guava/issues/<issue id>

Original comment by cgdecker@google.com on 1 Nov 2014 at 4:08

• Added labels: MigratedToGitHub

[GoogleCodeExporter]

Original comment by cgdecker@google.com on 3 Nov 2014 at 9:07

• Added labels: Restrict-AddIssueComment-Commit