Bug ID: JDK-6464365 FutureTask.{set,setException} not called by run()

Type: Bug
Component: core-libs
Sub-Component: java.util.concurrent
Affected Version: 6

Priority: P3
Status: Closed
Resolution: Fixed
OS: generic
CPU: generic

Submitted: 2006-08-26
Updated: 2011-03-07
Resolved: 2011-03-07

JDK 7
7 b07Fixed

Submitter of 6459119 writes:

In addition to this, I tried to do some proxying-magic with the actual provided instance of RunnableScheduledFuture. However, also here I found problems: The above-mentioned internal implementation of RunnableScheduledFuture extends FutureTask. This class uses an internal class called Sync which extends AbstractQueuedSynchronizer. The actual run() method invokes sync.innerRun(), which thus encapsulates any raised Exceptions (so that they can be set in the Future, obviously - but this makes this intervention-point useless too). But, yes, there is also a method setException(Throwable t), whose javadoc states "This method is invoked internally by the run method upon failure of the computation.". There is however no calls in any code to this method: when the contained task raises Exception whithin the innerRun() method, _inner_SetException() is invoked _directly_ (which most probably should have been a invocation to setException() instead, since this default does sync.innerSetException()). Further, there is also a "done()" method, which is invoked both on good and bad runs. But, again, there is no public ways to get to the Exception that was set with setException (or rather innerSetException).

EVALUATION Here's the complete fix: --- /u/martin/ws/dolphin/src/share/classes/java/util/concurrent/FutureTask.java 2007-01-06 19:37:20.242757000 -0800 +++ /u/martin/ws/FutureTask/src/share/classes/java/util/concurrent/FutureTask.java 2007-01-09 09:39:45.821536000 -0800 @@ -36,7 +36,7 @@ private final Sync sync; /** - * Creates a <tt>FutureTask</tt> that will upon running, execute the + * Creates a <tt>FutureTask</tt> that will, upon running, execute the * given <tt>Callable</tt>. * * @param callable the callable task @@ -49,7 +49,7 @@ } /** - * Creates a <tt>FutureTask</tt> that will upon running, execute the + * Creates a <tt>FutureTask</tt> that will, upon running, execute the * given <tt>Runnable</tt>, and arrange that <tt>get</tt> will return the * given result on successful completion. * @@ -161,6 +161,8 @@ private final class Sync extends AbstractQueuedSynchronizer { private static final long serialVersionUID = -7828117401763700385L; + /** State value representing that task is ready to run */ + private static final int READY = 0; /** State value representing that task is running */ private static final int RUNNING = 1; /** State value representing that task ran */ @@ -268,7 +270,6 @@ } if (compareAndSetState(s, RAN)) { exception = t; - result = null; releaseShared(0); done(); return; @@ -295,30 +296,30 @@ } void innerRun() { - if (!compareAndSetState(0, RUNNING)) + if (!compareAndSetState(READY, RUNNING)) return; try { runner = Thread.currentThread(); if (getState() == RUNNING) // recheck after setting thread - innerSet(callable.call()); + set(callable.call()); else releaseShared(0); // cancel } catch (Throwable ex) { - innerSetException(ex); + setException(ex); } } boolean innerRunAndReset() { - if (!compareAndSetState(0, RUNNING)) + if (!compareAndSetState(READY, RUNNING)) return false; try { runner = Thread.currentThread(); if (getState() == RUNNING) callable.call(); // don't set result runner = null; - return compareAndSetState(RUNNING, 0); + return compareAndSetState(RUNNING, READY); } catch (Throwable ex) { - innerSetException(ex); + setException(ex); return false; } } --- /u/martin/ws/dolphin/test/java/util/concurrent/FutureTask/Customized.java 1969-12-31 16:00:00.000000000 -0800 +++ /u/martin/ws/FutureTask/test/java/util/concurrent/FutureTask/Customized.java 2007-01-09 09:39:46.405864000 -0800 @@ -0,0 +1,190 @@ +/* + * @test 1.1 07/01/09 + * @bug 6464365 + * @summary Test state transitions; check protected methods are called + * @author Martin Buchholz + */ + +import java.util.*; +import java.util.concurrent.*; +import java.util.concurrent.atomic.*; + +public class Customized { + static final AtomicLong doneCount = new AtomicLong(0); + static final AtomicLong setCount = new AtomicLong(0); + static final AtomicLong setExceptionCount = new AtomicLong(0); + + static void equal(long expected, AtomicLong actual) { + equal(expected, actual.get()); + } + + static void equalCounts(long x, long y, long z) { + equal(x, doneCount); + equal(y, setCount); + equal(z, setExceptionCount); + } + + static class MyFutureTask<V> extends FutureTask<V> { + MyFutureTask(Runnable r, V result) { super(r, result); } + protected void done() { + doneCount.getAndIncrement(); + super.done(); + } + protected void set(V v) { + setCount.getAndIncrement(); + super.set(v); + } + protected void setException(Throwable t) { + setExceptionCount.getAndIncrement(); + super.setException(t); + } + public boolean runAndReset() { + return super.runAndReset(); + } + } + + static <V> void checkReady(final FutureTask<V> task) { + check(! task.isDone()); + check(! task.isCancelled()); + THROWS(TimeoutException.class, + new Fun(){void f() throws Throwable { + task.get(0L, TimeUnit.SECONDS); }}); + } + + static <V> void checkDone(final FutureTask<V> task) { + try { + check(task.isDone()); + check(! task.isCancelled()); + check(task.get() != null); + } catch (Throwable t) { unexpected(t); } + } + + static <V> void checkCancelled(final FutureTask<V> task) { + check(task.isDone()); + check(task.isCancelled()); + THROWS(CancellationException.class, + new Fun(){void f() throws Throwable { + task.get(0L, TimeUnit.SECONDS); }}, + new Fun(){void f() throws Throwable { + task.get(); }}); + } + + static <V> void checkThrew(final FutureTask<V> task) { + check(task.isDone()); + check(! task.isCancelled()); + THROWS(ExecutionException.class, + new Fun(){void f() throws Throwable { + task.get(0L, TimeUnit.SECONDS); }}, + new Fun(){void f() throws Throwable { + task.get(); }}); + } + + static <V> void cancel(FutureTask<V> task, boolean mayInterruptIfRunning) { + task.cancel(mayInterruptIfRunning); + checkCancelled(task); + } + + static <V> void run(FutureTask<V> task) { + boolean isCancelled = task.isCancelled(); + task.run(); + check(task.isDone()); + equal(isCancelled, task.isCancelled()); + } + + static void realMain(String[] args) throws Throwable { + final Runnable nop = new Runnable() { + public void run() {}}; + final Runnable bad = new Runnable() { + public void run() { throw new Error(); }}; + + try { + final MyFutureTask<Long> task = new MyFutureTask<Long>(nop, 42L); + checkReady(task); + equalCounts(0,0,0); + check(task.runAndReset()); + checkReady(task); + equalCounts(0,0,0); + run(task); + checkDone(task); + equalCounts(1,1,0); + equal(42L, task.get()); + run(task); + checkDone(task); + equalCounts(1,1,0); + equal(42L, task.get()); + } catch (Throwable t) { unexpected(t); } + + try { + final MyFutureTask<Long> task = new MyFutureTask<Long>(nop, 42L); + cancel(task, false); + equalCounts(2,1,0); + cancel(task, false); + equalCounts(2,1,0); + run(task); + equalCounts(2,1,0); + check(! task.runAndReset()); + } catch (Throwable t) { unexpected(t); } + + try { + final MyFutureTask<Long> task = new MyFutureTask<Long>(bad, 42L); + checkReady(task); + run(task); + checkThrew(task); + equalCounts(3,1,1); + run(task); + equalCounts(3,1,1); + } catch (Throwable t) { unexpected(t); } + + try { + final MyFutureTask<Long> task = new MyFutureTask<Long>(nop, 42L); + checkReady(task); + task.set(99L); + checkDone(task); + equalCounts(4,2,1); + run(task); + equalCounts(4,2,1); + task.setException(new Throwable()); + checkDone(task); + equalCounts(4,2,2); + } catch (Throwable t) { unexpected(t); } + + try { + final MyFutureTask<Long> task = new MyFutureTask<Long>(nop, 42L); + checkReady(task); + task.setException(new Throwable()); + checkThrew(task); + equalCounts(5,2,3); + run(task); + equalCounts(5,2,3); + task.set(99L); + checkThrew(task); + equalCounts(5,3,3); + } catch (Throwable t) { unexpected(t); } + + System.out.printf("doneCount=%d%n", doneCount.get()); + System.out.printf("setCount=%d%n", setCount.get()); + System.out.printf("setExceptionCount=%d%n", setExceptionCount.get()); + } + + //--------------------- Infrastructure --------------------------- + static volatile int passed = 0, failed = 0; + static void pass() {passed++;} + static void fail() {failed++; Thread.dumpStack();} + static void fail(String msg) {System.out.println(msg); fail();} + static void unexpected(Throwable t) {failed++; t.printStackTrace();} + static void check(boolean cond) {if (cond) pass(); else fail();} + static void equal(Object x, Object y) { + if (x == null ? y == null : x.equals(y)) pass(); + else fail(x + " not equal to " + y);} + public static void main(String[] args) throws Throwable { + try {realMain(args);} catch (Throwable t) {unexpected(t);} + System.out.printf("%nPassed = %d, failed = %d%n%n", passed, failed); + if (failed > 0) throw new AssertionError("Some tests failed");} + private static abstract class Fun {abstract void f() throws Throwable;} + static void THROWS(Class<? extends Throwable> k, Fun... fs) { + for (Fun f : fs) + try { f.f(); fail("Expected " + k.getName() + " not thrown"); } + catch (Throwable t) { + if (k.isAssignableFrom(t.getClass())) pass(); + else unexpected(t);}} +}

09-01-2007

SUGGESTED FIX See Eval

30-10-2006

EVALUATION I had trouble understanding the submitter's intent. Let me rephrase that. This program: import java.io.*; import java.util.*; import java.util.concurrent.*; import java.util.concurrent.atomic.*; public class SetException { static final AtomicLong setExceptionCount = new AtomicLong(0); static class MyFutureTask<V> extends FutureTask<V> { MyFutureTask(Runnable r, V result) { super(r, result); } protected void setException(Throwable t) { setExceptionCount.getAndIncrement(); super.setException(t); } } public static void main(String[] args) throws Throwable { new MyFutureTask<Long>( new Runnable() { public void run() { throw new Error(); }}, 42L) .run(); System.out.printf("setExceptionCount=%d%n", setExceptionCount.get()); } } prints setExceptionCount=0 instead of setExceptionCount=1 Here's the obvious fix (at least for setException): --- /tmp/geta20548 2006-08-26 14:31:43.357959200 -0700 +++ FutureTask.java 2006-08-26 14:19:23.845263000 -0700 @@ -300,27 +300,27 @@ try { runner = Thread.currentThread(); if (getState() == RUNNING) // recheck after setting thread innerSet(callable.call()); else releaseShared(0); // cancel } catch (Throwable ex) { - innerSetException(ex); + setException(ex); } } boolean innerRunAndReset() { if (!compareAndSetState(0, RUNNING)) return false; try { runner = Thread.currentThread(); if (getState() == RUNNING) callable.call(); // don't set result runner = null; return compareAndSetState(RUNNING, 0); } catch (Throwable ex) { - innerSetException(ex); + setException(ex); return false; } } } }

26-08-2006