There are a number of protected hook methods that execute after some
event has occurred, which may also be waited for by some threads.
It is not clear whether the hook methods should execute before the
threads are released or not.  It's a tradeoff of concurrency vs.
predictability.  The current behavior is underdocumented and undertested.
It's too late for the behavior to be changed.

Here are the method pairs:

FutureTask.get()
FutureTask.done()

ThreadPoolExecutor.awaitTermination()
ThreadPoolExecutor.terminated()

CyclicBarrier.await()
barrier action

Any more?

Extract from email follows:

Hi Folks,

We recently started running our tests on a faster multi-processor
machine and ran into a few race conditions, one of which we're unsure
of how to fix.  What we're experiencing is that a Thread that has
created a future and calls 'get' on it can retrieve the result before
the future has 'done' called on itself.  This leads to more code being
able to act before the finished-state of the future has cleaned itself
up.

The reason this is a problem is that we want to limit the number of
outgoing pings sent to a host.  When Manager.ping(Host) is called, it
synchronizes on a map and checks to see if there's an outstanding
future for that host, and if so, returns it.  Otherwise (if there's no
outstanding future), it creates one, adds it to the map, submits it to
an executor service, and returns it.  When the future is finished,
it's overridden done method synchronizes on the map and removes
itself.

The end result is the following code sometimes works & sometimes doesn't:

---
  // Setup response scenario, send ping & assert response
  Future<PingResult> future = pingManager.ping(host);
  PingResult result = future.get();
  // check response assertions
  // Change scenario so no response is sent..
  try {
      pingManager.ping(host).get();
      fail("shouldn't have gotten a response!");
      // Note: this fails occasionally because the above
      // future's done method isn't called yet (which removes
      // the outstanding future), so the pingManager
      // is returning the same Future as above.
  } catch(ExcecutionException expected) {}
---

We can easily add a small sleep, or a Thread.yield() to make sure
other threads process (and the done() is called), but I'm wondering if
there's any better way.

Thanks very much for any ideas.

Sam


Sam,

done() is called after the internal synchronization used to block get() is
released, so yes the thread that was doing get() can proceed to execute code
for a long time before done() actually gets to be executed.

The timing of these hook methods is always somewhat subjective. In some ways
it might have been better to invoke done() prior to the actual release - in
the same way that a barrier action for a CyclicBarrier is executed prior to
the release of the barrier.

The only way I can think to achieve what you want is to override all the get
methods to add a "waitForDone" call that blocks until your done() method
unblocks it. This is a bit crude but can be easily done with a
CountDownLatch:

    class MyFutureTask extends FutureTask {

       CountDownLatch done = new CountDownLatch(1);
       public V get() throws ... {
          try { return super.get(); }
          finally {
             done.await();
          }
        }

       // timed get() is a bit trickier :)

       protected void done() {
          try {
             // real stuff
          }
          finally {
             done.countDown();
          }
      }
  }

Cheers,
David Holmes

The spec doesn't make it clear what order things happen so there is at least
room for a clarification. Changing the current behaviour is not an option,
but perhaps there could be some way to control it. It isn't uncommon for
"bookkeeping" tasks to occur asynchronously with respect to the main
behaviour - it all depends on whether that bookkeeping is independent of
that main behaviour, which in your case it is not. Noone else has raised
this issue to date so that says something :)

Cheers,
David Holmes


Fair enough.  :)

The CountDownLatch option is probably the best way to go overall, as
it lets the individual FutureTask subclasses control whether or not
done() stalls a get().

I do suggest documentating the asynchronous  customer  of get/done.  The
current documentation leaves it untouched, which depending on the
reader's viewpoint could mean many different things.  Clarifying that
'done' may not be called prior to 'get' completing may lead more
people to recognize the issue and take care of it before it bites.

Sam
Posted Date : 2007-01-31 18:58:07.0

N/A

Yes.
Posted Date : 2007-01-31 18:58:45.0

Sam adds the very instructive comment:

"As we investigated the CountDownLatch, we noticed that that allowing
get() to complete prior to done() is a very useful feature.  Many
done() implementations will want to query get() to perform the
bookkeeping based on the result.  If get() required done() to
complete, then there would be a deadlock whenever get() was called
within done().  So, the current behavior does seem to be the best
choice overall, though documenting it would be even better."
Posted Date : 2007-01-31 20:43:35.0