JDK-8073704 : FutureTask.isDone returns true when task has not yet completed
  • Type: Bug
  • Component: core-libs
  • Sub-Component: java.util.concurrent
  • Affected Version: 7,8u20,9
  • Priority: P4
  • Status: Closed
  • Resolution: Duplicate
  • Submitted: 2015-02-10
  • Updated: 2016-06-13
  • Resolved: 2015-09-21
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 9
9 b88Resolved
Related Reports
Duplicate :  
Description
FULL PRODUCT VERSION :


ADDITIONAL OS VERSION INFORMATION :
really doesn't matter :P

A DESCRIPTION OF THE PROBLEM :
implementation of FutureTask#isDone is as follows :

public boolean isDone() {
        return state != NEW;
}

while it should be as follows : 

public boolean isDone(){
         return state != NEW && state != COMPLETING && state != INTERRUPTING;
}

or (less code, but less readable):

public boolean isDone(){
         return state > COMPLETING && state != INTERRUPTING;
}

STEPS TO FOLLOW TO REPRODUCE THE PROBLEM :
open sources, go to FutureTask#isDone implementation, read javadoc of Future interface, look at them.

EXPECTED VERSUS ACTUAL BEHAVIOR :
EXPECTED -
either

public boolean isDone(){
         return state != NEW && state != COMPLETING && state != INTERRUPTING;
}

or:

public boolean isDone(){
         return state > COMPLETING && state != INTERRUPTING;
}
ACTUAL -
public boolean isDone() {
        return state != NEW;
}

REPRODUCIBILITY :
This bug can be reproduced always.

CUSTOMER SUBMITTED WORKAROUND :
Write your own RunnableFuture implementation with long javadoc explaining.


Comments
Committed to jsr166 CVS - will appear in openjdk9 when the Great Sync happens, or earlier if someone takes on the task.
05-03-2015

Sure; please go ahead, in the spirit of pendantic compliance.
04-03-2015

Thanks go to Paul for the repro and David for the suggested fix. I intend to commit the variant below to jsr166 CVS. Can Doug give his blessing? --- src/main/java/util/concurrent/FutureTask.java 4 Jan 2015 09:15:11 -0000 1.110 +++ src/main/java/util/concurrent/FutureTask.java 4 Mar 2015 20:03:26 -0000 @@ -376,19 +376,20 @@ WaitNode q = null; boolean queued = false; for (;;) { - if (Thread.interrupted()) { - removeWaiter(q); - throw new InterruptedException(); - } - int s = state; if (s > COMPLETING) { if (q != null) q.thread = null; return s; } - else if (s == COMPLETING) // cannot time out yet + else if (s == COMPLETING) + // We may have already promised (via isDone) that we are done + // so never return empty-handed or throw InterruptedException Thread.yield(); + else if (Thread.interrupted()) { + removeWaiter(q); + throw new InterruptedException(); + } else if (q == null) { if (timed && nanos <= 0L) return s;
04-03-2015

I think the simplest/best way is to check the state before checking for interruption: for (;;) { - if (Thread.interrupted()) { - removeWaiter(q); - throw new InterruptedException(); - } int s = state; if (s > COMPLETING) { if (q != null) q.thread = null; return s; } else if (s == COMPLETING) // cannot time out yet Thread.yield(); else + if (Thread.interrupted()) { + removeWaiter(q); + throw new InterruptedException(); + } else if (q == null) { if (timed && nanos <= 0L) return s; q = new WaitNode(); } elseif ...
04-03-2015

I've changed my mind. If isDone() returns true, then get() should surely return the value without throwing IE. Exactly how best to achieve that is unclear. Most obviously, have isDone return when > COMPLETING. Alternatively, get() could spin-wait when status == COMPLETING. We may even rewrite state maintenance to be more like CompletableFuture.
04-03-2015

It is common in j.u.c to give preference to detecting interrupts as cancellation points are relatively rare so it is important to communicate that the interruption occurred. The interruption of the current thread is not an indication of the completion or otherwise of the FutureTask itself. The task may still be done but an interrupted thread doing get() may throw InterruptedException. Arguably we could give preference to returning the result, by moving the interruption check later in the loop within awaitDone.
27-02-2015

Just for fun here is a pathological case where one can observe something: public static void main(String[] args) throws Exception { AtomicReference<FutureTask<Integer>> a = new AtomicReference<>(); Runnable task = () -> { while (true) { FutureTask<Integer> f = new FutureTask<>(() -> 1); a.set(f); f.run(); } }; Supplier<Runnable> observe = () -> () -> { while (a.get() == null); int c = 0; int ic = 0; while (true) { c++; FutureTask<Integer> f = a.get(); while (!f.isDone()) {} try { /* Set the interrupt flag of this thread. The future reports it is done but in some cases a call to "get" will result in an underlying call to "awaitDone" if the state is observed to be completing. "awaitDone" checks if the thread is interrupted and if so throws an InterruptedException. */ Thread.currentThread().interrupt(); f.get(); } catch (ExecutionException e) { throw new RuntimeException(e); } catch (InterruptedException e) { ic ++; System.out.println("InterruptedException observed when isDone() == true " + c + " " + ic + " " + Thread.currentThread()); } } }; CompletableFuture.runAsync(task); Stream.generate(observe::get) .limit(Runtime.getRuntime().availableProcessors() - 1) .forEach(CompletableFuture::runAsync); Thread.sleep(1000); System.exit(0); }
26-02-2015

It's intentional.
26-02-2015

The current implementation is intentional, for the reasons David noted. At minimum, the submitter should explain why they think a change is desirable.
26-02-2015

I think I agree with Paul's suggested change, but this is a very minor issue and I'm not sure that there are any observable affects from it.
26-02-2015

The check for INTERRUPTING is not needed, once the task is in that state the result is a CancellationException (see isCancelled and get methods). There is some slight jitter in the reporting by isDone and the transition of the state from NEW to something > COMPLETING, but it does not seem a big issue. I suppose it could be changed to: public boolean isDone() { return state > COMPLETING; } Unless i have missed something subtle about the interactions.
26-02-2015

I do not think any change is necessary. The COMPLETING state is a transient one which indicates that we are in fact done, we just haven't finished setting the final state yet. If you query how we are "done" (eg via get()) you will get the correct answer. The transient state is not observable to the programmer.
26-02-2015

The description of method public boolean isDone() need to be checked. The method states - "Returns true if this task completed. Completion may be due to normal termination, an exception, or cancellation -- in all of these cases, this method will return true." url: https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/FutureTask.html
24-02-2015