views:

880

answers:

4

I am currently hunting a nasty bug in a multi-threaded environment using FutureTasks and Executors. The basic idea is this to have a fixed number of threads execute individual FutureTasks that compute a result that is to be displayed in a a table (never mind the GUI aspect here).

I have been looking at this for so long, I am beginning to doubt my sanity.

Consider this piece of code:

public class MyTask extends FutureTask<Result> {
   private String cellId;
   ...
   protected void done() {
      if (isCancelled()) return;
      try {
          Result r = get(); // should not wait, because we are done
          ... // some processing with r
          sendMessage(cellId, r);
      } catch (ExecutionException e) { // thrown from get
         ... 
      } catch (InterruptedException e) { // thrown from get
         ... 
      }
   }
   ...
}

When done() is called by an Executor handling an instance of MyTask, I check if I got there, because the task was cancelled. If so, I skip all remaining activities, especially I do not call sendMessage().

The documentation for FutureTask.done() says:

Protected method invoked when this task transitions to state isDone (whether normally or via cancellation). The default implementation does nothing. Subclasses may override this method to invoke completion callbacks or perform bookkeeping. Note that you can query status inside the implementation of this method to determine whether this task has been cancelled. (API Reference)

But what I do not get from the documentation of FutureTask are the semantics while done() is being executed. What if I pass the isCancelled() check at the beginning, but right after that some other thread calls my cancel() method? Will that cause my task to change its mind and reply isCancelled() == true from then on?

If so, how would I later know if the the message was sent? Looking at isDone() would just tell me that execution of the task was finished, but as isCancelled() were true then as well, I could not know if it got to send the message out in time.

Maybe this is obvious, but I do not really see it right now.

+2  A: 

From the API (emphasis mine):

public boolean cancel(boolean mayInterruptIfRunning)

Description copied from interface: Future

Attempts to cancel execution of this task. This attempt will fail if the task has already completed, already been cancelled, or could not be cancelled for some other reason.

So FutureTask is working off the assumption that you cannot cancel a task when it has transitioned to the isDone stage.

I wrote a little test (as suggested by Aaron Digulla) - and in fact, once isDone() is entered, the cancel() call does not change the state anymore.
Daniel Schneller
A: 

I suggest to write a small test case which allows you to call cancel() while your Future instance hangs in done() and see what happens.

Aaron Digulla
+1  A: 

Why not send the message "outside" of the task, based on the outcome of the Future<V> object returned by an ExecutorService? I've used this pattern and it seems to work well: Submit a bunch of Callable<V> tasks through an ExecutorService. Then, for each primary task, submit a secondary task that waits on the Future<V> of the primary task and does some follow-up action (like send a message) only if the Future<V> indicates that the primary task completed successfully. There is no guesswork with this approach. When the call to Future<V>.get() returns, you're guaranteed that the task has reached a terminal state, as long as you don't call the version of get that takes a timeout argument.

If you take this approach, you should use two separate ExecutorService instances: one for the primary tasks and one for the secondary ones. This is to prevent deadlocks. You don't want secondary tasks to start up and potentially block primary tasks from starting when the thread pool size is limited.

There's no need to extend FutureTask<V> at all. Just implement your tasks as Callable<V> objects. But if for some reason you want to detect if the task was canceled from within the Callable<V> code, just check the interrupt status of the thread with Thread.interrupted().

Rob H
A: 

FutureTask#done() is called no more than once for any given instance, and it's only called for one reason -- run() completed either with or without error, or cancel() ran before either of the preceding events occurred. The record of completion by any of these outcomes is latching. The reason a FutureTask completed can't change, regardless of competing events seemingly happening "at the same time".

Hence, within FutureTask#done() only one of isCancelled() or isDone() will return true then and forever more. It's difficult to distinguish between isDone() reporting true by way of error or successful completion. You can't override set() or setException(Throwable) decisively, as both delegate to the inner AQS to decide whether the attempt to record a successful yielding of a value or encountering an exception should stick. Overriding either method only lets you know that it was called, but you can't observe the decision made by the base implementation. If either event occurs "too late" -- say, after cancellation -- the attempt to record the value or the exception will be ignored.

Studying the implementation, the only way I see to discern a non-canceled successful outcome from an error is to bite the bullet and call get().

seh