JDK-6459119 : Explain how afterExecute can access a submitted job's Throwable
  • Type: Bug
  • Component: core-libs
  • Sub-Component: java.util.concurrent
  • Affected Version: 6
  • Priority: P3
  • Status: Closed
  • Resolution: Fixed
  • OS: windows_xp
  • CPU: x86
  • Submitted: 2006-08-10
  • Updated: 2011-05-17
  • Resolved: 2011-05-17
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 7
7 b08Fixed
Related Reports
Relates :  
Relates :  
Description
FULL PRODUCT VERSION :
java version "1.6.0-beta2"
Java(TM) SE Runtime Environment (build 1.6.0-beta2-b86)
Java HotSpot(TM) Client VM (build 1.6.0-beta2-b86, mixed mode, sharing)

ADDITIONAL OS VERSION INFORMATION :
Microsoft Windows XP [Version 5.1.2600]

A DESCRIPTION OF THE PROBLEM :
ScheduledThreadPoolExecutor swallows Exceptions coming from Runnables' run() method submitted via execute(Runnable command), in contrast to ThreadPoolExecutor's same method, where afterExecute(Runnable r, Throwable t) will be invoked with the RuntimeException (not Errors, see bug #6450211) that happened in such cases.

This is because ScheduledTPE wraps the Runnable in a ScheduledFutureTask, by submitting it to schedule(command, 0, TimeUnit.NANOSECONDS), however it returns void, thus there is no way to get at the Exception thrown from the Runnable's run() method.

Actually, it seems like afterExecute(...) never will get an non-null Throwable in the ScheduledTPE, and it will probably also count all tasks as "completed", opposed to normal TPE, which distinguishes here (but this might also be wrong, confer bug #6450207).

This seems inconsistent, and I haven't found a description of this in the javadoc. It would seem more correct to let afterExecute also know of all Throwables that happened in a ScheduledTPE, in particular since this seems to be the only way to "install" an (external) "exception listener" in the ThreadPool.

STEPS TO FOLLOW TO REPRODUCE THE PROBLEM :
run test case

EXPECTED VERSUS ACTUAL BEHAVIOR :
EXPECTED -
ThreadPool Runnable's Exception caught!
(Pool Thread's stacktrace, due to unwind)
ScheduledThreadPool Runnable's Exception caught!
(Maybe Pool Thread's stacktrace, due to unwind?)

ACTUAL -
ThreadPool Runnable's Exception caught!
(Pool Thread's stacktrace, due to unwind)
ScheduledThreadPool Runnable gave no Exception

REPRODUCIBILITY :
This bug can be reproduced always.

---------- BEGIN SOURCE ----------
import java.util.concurrent.LinkedBlockingQueue;
import java.util.concurrent.ScheduledThreadPoolExecutor;
import java.util.concurrent.ThreadPoolExecutor;
import java.util.concurrent.TimeUnit;

public class DifferentHandling {

	public static void main(String[] args) {
		ThreadPool threadPool = new ThreadPool();
		ScheduledThreadPool scheduledThreadPool = new ScheduledThreadPool();
		threadPool.execute(new Task());
		scheduledThreadPool.execute(new Task());
	}

	private static class Task implements Runnable {
		public void run() {
			throw new RuntimeException("test");
		}
	}
	
	private static class ThreadPool extends ThreadPoolExecutor {
		ThreadPool() {
			super(10, 10, 0L, TimeUnit.MILLISECONDS, new LinkedBlockingQueue<Runnable>());
		}

		@Override
		protected void afterExecute(Runnable r, Throwable t) {
			if (t != null) {
				System.out.println("ThreadPool Runnable's Exception caught!");
			}
			else {
				System.out.println("ThreadPool Runnable's gave no Exception");
			}
		}
	}

	private static class ScheduledThreadPool extends ScheduledThreadPoolExecutor {
		ScheduledThreadPool() {
			super(10);
		}

		@Override
		protected void afterExecute(Runnable r, Throwable t) {
			if (t != null) {
				System.out.println("ScheduledThreadPool Runnable's Exception caught!");
			}
			else {
				System.out.println("ScheduledThreadPool Runnable gave no Exception");
			}
		}
	}
}

---------- END SOURCE ----------

CUSTOMER SUBMITTED WORKAROUND :
Possibly implement the decorateTask(Runnable runnable, RunnableScheduledFuture<V> task) to wrap the task in a new RunnableScheduledFuture which invokes directly through on all methods except run, which stores any Exception in a static ThreadLocal before throwing on, and then in afterExecute check for (and clear) this.
The submitter added the following SDN comment:

My point is exactly that afterExecute _cannot_ be used - the Exception argument when it is being invoked is always null (since it is wrapped up in an internal implementation of (Runnable)ScheduledFuture which _don't_ propagate any Exceptions back to be included in the afterExecute invocation). The actual problem is that if you just want to send off a task to be executed (or scheduled), and don't want to hang on to the returned Future to see what happens, any Exceptions or Errors happening within the run()/call() are just utterly swallowed without any trace, and I havent found any natural extensionpoint in the class to handle this (which leads me to believe that afterExecute isn't working as (probably?) intended in ScheduledTPE).

(Might be another bug:) In addition, I find it hard to see how one easily can employ the decorateTask methods: The RunnableScheduledFuture that you're provided with is an internal (non-static) class of ScheduledThreadPoolExecutor  (called ScheduledFutureTask), without public accessors of the internal data. This combined with the fact that decorateTask methods do not provide all the information that was given to the actual schedule or submit invocation (delay, initialDelay, period, unit and result) makes it hard (I so far think it is impossible) to make an external implementation that will honor the contracts of the schedule-methods. The methods run() and its private sibling runPeriodic() are problematic to implement externally due to this lacking information, and in addition the  accessed private field sequencer (and NANO_ORIGIN) of ScheduledTPE might make it even harder (at least to just copy'n'paste implement directly!).

(Might be yet another bug:) 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).

The workaround-suggestion I came up with in "CUSTOMER SUBMITTED WORKAROUND" is obviously flawed, since the whole idea of a Worker-Pool is that _some other_ thread should run the run()/call() method, hence ThreadLocals won't cut it!

However, after hours of banging my head while looking in vain for appropriate extension-points or listener-registration possibilities, I finally came up with an (obvious?) extension that simply overrides all submit-methods by wrapping up the submitted Callable or Runnable in a new instance of the same, which invokes the run/call in a try-catch, and runs notification to registered TaskExceptionListeners before throwing on (which hopefully should preserve contract - any Callables or Runnables laying in the queue are already "hidden" behind the internal RunnableScheduledFuture)

/**
 * Extension of {@link ScheduledThreadPoolExecutor} that makes it possible to install {@link TaskExceptionListener
 * TaskExceptionListeners} to get notified of Exceptions and Errors that happen while the
 * Runnable or Callable is executed.
 * 
 * @author endre
 */
public class ScheduledThreadPoolExecutorWithExceptionListening extends ScheduledThreadPoolExecutor {

	static final Logger log = Logger.getLogger("picorg.util.ScheduledExecutor");

	public ScheduledThreadPoolExecutorWithExceptionListening(int corePoolSize, RejectedExecutionHandler handler) {
		super(corePoolSize, handler);
	}

	public ScheduledThreadPoolExecutorWithExceptionListening(int corePoolSize, ThreadFactory threadFactory,
			RejectedExecutionHandler handler) {
		super(corePoolSize, threadFactory, handler);
	}

	public ScheduledThreadPoolExecutorWithExceptionListening(int corePoolSize, ThreadFactory threadFactory) {
		super(corePoolSize, threadFactory);
	}

	public ScheduledThreadPoolExecutorWithExceptionListening(int corePoolSize) {
		super(corePoolSize);
	}

	private final CopyOnWriteArraySet _taskExceptionListeners = new CopyOnWriteArraySet();

	public void addTaskExceptionListener(TaskExceptionListener tel) {
		_taskExceptionListeners.add(tel);
	}

	/**
	 * "Rename-method" of
	 * {@link #addTaskExceptionListener(com.picorg.util.ScheduledThreadPoolExecutorWithExceptionListening.TaskExceptionListener)},
	 * so that it's easier to DI the {@link TaskExceptionListener}.
	 * 
	 * @param tel the TaskExceptionListener to add.
	 */
	public void setTaskExceptionListener(TaskExceptionListener tel) {
		_taskExceptionListeners.add(tel);
	}

	public void removeTaskExceptionListener(TaskExceptionListener tel) {
		_taskExceptionListeners.remove(tel);
	}

	private void exception(Runnable r, Throwable t) {
		for (TaskExceptionListener listener : _taskExceptionListeners) {
			listener.taskRaisedException(r, t);
		}
	}

	private void exception(Callable c, Throwable t) {
		for (TaskExceptionListener listener : _taskExceptionListeners) {
			listener.taskRaisedException(c, t);
		}
	}

	private Runnable newRunnable(final Runnable command) {
		return new Runnable() {
			public void run() {
				try {
					command.run();
				}
				catch (RuntimeException e) {
					exception(command, e);
					throw e;
				}
				catch (Error e) {
					exception(command, e);
					throw e;
				}
			}
		};
	}

	private  Callable newCallable(final Callable callable) {
		return new Callable() {
			public T call() throws Exception {
				try {
					return callable.call();
				}
				catch (Exception e) {
					exception(callable, e);
					throw e;
				}
				catch (Error e) {
					exception(callable, e);
					throw e;
				}
			}
		};
	}

	@Override
	public  ScheduledFuture schedule(Callable callable, long delay, TimeUnit unit) {
		if (log.isTraceEnabled())
			log.trace("threadpool.schedule(Callable:[" + callable + "], delay:[" + delay + ' ' + unit + "]).");
		return super.schedule(newCallable(callable), delay, unit);
	}

	@Override
	public ScheduledFuture schedule(Runnable command, long delay, TimeUnit unit) {
		if (log.isTraceEnabled())
			log.trace("threadpool.schedule(Runnable:[" + command + "], delay:[" + delay + ' ' + unit + "]).");
		return super.schedule(newRunnable(command), delay, unit);
	}

	@Override
	public ScheduledFuture scheduleAtFixedRate(Runnable command, long initialDelay, long period, TimeUnit unit) {
		if (log.isTraceEnabled())
			log.trace("threadpool.scheduleAtFixedRate(Runnable:[" + command + "], initialDelay:[" + initialDelay
					+ "], period:[" + period + ' ' + unit + "]).");
		return super.scheduleAtFixedRate(newRunnable(command), initialDelay, period, unit);
	}

	@Override
	public ScheduledFuture scheduleWithFixedDelay(Runnable command, long initialDelay, long delay, TimeUnit unit) {
		if (log.isTraceEnabled())
			log.trace("threadpool.scheduleWithFixedDelay(Runnable:[" + command + "], initialDelay:[" + initialDelay
					+ "], period:[" + delay + ' ' + unit + "]).");
		return super.scheduleWithFixedDelay(newRunnable(command), initialDelay, delay, unit);
	}

	/**
	 * Listener for Exceptions to submitted and scheduled Runnables and Callables.
	 * 
	 * @author endre
	 */
	public interface TaskExceptionListener extends EventListener {
		void taskRaisedException(Runnable r, Throwable t);

		void taskRaisedException(Callable c, Throwable t);
	}

	/**
	 * A simple Logging implementation of {@link TaskExceptionListener}.
	 * 
	 * @author endre
	 */
	public static class LoggingTaskExceptionListener implements TaskExceptionListener {

		public void taskRaisedException(Runnable r, Throwable t) {
			log.warn("submitted/scheduled Runnable [" + r + "] raised Exception while running.", t);
		}

		public void taskRaisedException(Callable c, Throwable t) {
			log.warn("submitted/scheduled Callable [" + c + "] raised Exception while running.", t);
		}
	}
}

Comments
EVALUATION I don't think we can make the submitter entirely happy, but we have resolved 6464365: FutureTask.{set,setException} not called by run() and we are modifying the spec for TPE.afterExecute to add * <p><b>Note:</b> When actions are enclosed in tasks (such as * {@link FutureTask}) either explicitly or via methods such as * {@code submit}, these task objects catch and maintain * computational exceptions, and so they do not cause abrupt * termination, and the internal exceptions are <em>not</em> * passed to this method. If you would like to trap both kinds of * failures in this method, you can further probe for such cases, * as in this sample subclass that prints either the direct cause * or the underlying exception if a task has been aborted: * * <pre> {@code * class ExtendedExecutor extends ThreadPoolExecutor { * // ... * protected void afterExecute(Runnable r, Throwable t) { * super.afterExecute(r, t); * if (t == null && r instanceof Future<?>) { * try { * Object result = ((Future<?>) r).get(); * } catch (CancellationException ce) { * t = ce; * } catch (ExecutionException ee) { * t = ee.getCause(); * } catch (InterruptedException ie) { * Thread.currentThread().interrupt(); // ignore/reset * } * } * if (t != null) * System.out.println(t); * } * }}</pre> and we are adding to STPE.execute: * <p>A consequence of the use of {@code ScheduledFuture} objects is * that {@link ThreadPoolExecutor#afterExecute afterExecute} is always * called with a null second {@code Throwable} argument, even if the * {@code command} terminated abruptly. Instead, the {@code Throwable} * thrown by such a task can be obtained via {@link Future#get}. *
30-01-2007

EVALUATION Submitter added the following SDN comment: Submitter again: just wanted to point out to other visitors of this bug that the "Might be yet another bug" comment (the one about setException fubarred) is filed as bug 6464365 by Martin Buchholz, one of concurrent's maintainers. However, there is still no comment about the decoration-problem - apparently these absolutely useless decorating-methods are as good as they'll ever get. And I do still not agree with the maintainers: the STPE as it stands now is very prone to silent errors all over that will be amazingly hard to track down for developers using it. Any Throwable of any kind will be swallowed without a hitch; there just NO sign of them at all in the default mode (in stark contrast to the TPE, which STPE directly extends), and you need at least 15 lines of boilerplate code to catch them, instead of the _one_ line the afterExecute method's signature invites you to believe. .. and the proposed javadoc is wrong: the "@param t the exception that caused termination, or null if execution completed normally." is a lie: it is ALWAYS null, and can NEVER be anything else in STPE, which is what I find rather .. weird.
19-10-2006

EVALUATION Submitter writes: "I don't mean putting anything "into afterExecute", but into the code _that invokes_ afterExecute (which happens to be ThreadPoolExecutor$Worker.runTask()). There is several ways that the code that invokes afterExecute could be changed to pick up the Throwable that was raised by the submitted Runnable/Callable's run/call, in particular since the implementation of RunnableScheduledFuture that ScheduledTPE encapsulates the submitted Runnable/Callable in (SheduledThreadPoolExecutor$ScheduledFutureTask) already is an inner class of ScheduledTPE, and could thus override/augment anything. I don't quite get your comment about instanceof being a "weird non-Java" thing: There is three instanceof's within ScheduledTPE already, checking whether the Runnable at hand is the internal ScheduledFutureTask or the interface RunnableScheduledFuture. The original TPE's inner Worker class could do such a instanceof check within its runTask() method (which is the method that invokes task.run(), obviously), and if it is a ScheduledFutureTask, and it contains a Throwable, it could pick it out and invoke afterExecute with it. Or the entire Worker class could be implemented afresh in ScheduledTPE. I don't see this being an incompatible change, currently the always-null argument contains no info, thus most probably no-one is checking against it, while it would change to contain any Throwable happening within run/call." We should probably be having a private email exchange here. Unfortunately in the modern world people are afraid to reveal their identity. Sigh. I'm martin dot buchholz at sun dot com , please email directly if you wish. I disagree that the proposed change would be compatible. We went to the trouble of specifying that the throwable trapped inside a FutureTask is *not* passed to afterExecute. "Note: When actions are enclosed in tasks (such as FutureTask) either explicitly or via methods such as submit, these task objects catch and maintain computational exceptions, and so they do not cause abrupt termination, and the internal exceptions are not passed to this method." Regarding instanceof: We do occasionally use them, but we try to avoid them. Our suspicion level goes way up, however, when thinking about instanceof tests for nested classes of a subclass.
26-08-2006

EVALUATION Submitter asks: "But I reckon it is possible to change the afterExecute invocation code in the ScheduledTPE to actually include the Throwable, so that they aren't handled this differently - isn't that a better option here, rather than this rather implementation-specific instanceof use?" But putting instanceof tests into afterExecute would be a weird non-Java thing to do, and would be an incompatible change. We've already made it mostly clear in the docs that afterExecute cannot work that way.
22-08-2006

EVALUATION Adding an example to ThreadPoolExecutor.afterExecute should clarify this. Recasting this bug as a doc bug. Doug writes: How about: /** * Method invoked upon completion of execution of the given Runnable. * This method is invoked by the thread that executed the task. If * non-null, the Throwable is the uncaught <tt>RuntimeException</tt> * or <tt>Error</tt> that caused execution to terminate abruptly. * * <p>This implementation does nothing, but may be customized in * subclasses. Note: To properly nest multiple overridings, subclasses * should generally invoke <tt>super.afterExecute</tt> at the * beginning of this method. * * <p><b>Note:</b> When actions are enclosed in tasks (such as * {@link FutureTask}) either explicitly or via methods such as * <tt>submit</tt>, these task objects catch and maintain * computational exceptions, and so they do not cause abrupt * termination, and the internal exceptions are <em>not</em> * passed to this method. If you would like to trap both kinds of * failures in this method, you can further probe for such cases, * as in this sample subclass that prints either the direct cause * or the underlying exception if a task has been aborted: * * <pre> * class ExtendedExecutor extends ThreadPoolExecutor { * // ... * protected void afterExecute(Runnable r, Throwable t) { * super.afterExecute(r, t); * if (t == null && r instanceof Futuree&lt;?&gt;) { * try { * Object result = ((Future&lt;?&gt;) r).get(); * } catch (CancellationException ce) { * t = ce; * } catch (ExecutionException ee) { * t = ee.getCause(); * } catch (InterruptedException ie) { * Thread.currentThread().interrupt(); // ignore/reset * } * } * if (t != null) * System.out.println(t); * } * } * </pre> * * @param r the runnable that has completed. * @param t the exception that caused termination, or null if * execution completed normally. */ protected void afterExecute(Runnable r, Throwable t) { }
18-08-2006

EVALUATION It *is* possible to get at the exception thrown by a task using afterExecute: (this should be clarified in the documentation) ----------------------------------------------------- import java.util.*; import java.util.concurrent.*; public class STPE extends ScheduledThreadPoolExecutor { STPE() { super(1); } protected void afterExecute(Runnable r, Throwable t) { if (r instanceof Future<?>) { try { Object result = ((Future<?>) r).get(); } catch (InterruptedException ie) { System.out.println("interrupted!"); } catch (ExecutionException ee) { Throwable realThrowable = ee.getCause(); System.out.println("threw!"); realThrowable.printStackTrace(); } catch (CancellationException ce) { System.out.println("cancelled!"); } } System.out.println(r); System.out.println(t); System.out.println("-----------------------"); } public static void main(String[] args) throws Throwable { STPE pool = new STPE(); pool.submit(new Runnable() { public void run() { throw new Error(); }}); ScheduledFuture<?> sf = pool.schedule( new Runnable() { public void run() { try { Thread.sleep(10*1000); } catch (Throwable t) {}}}, 0L, TimeUnit.SECONDS); Thread.sleep(1 * 1000); sf.cancel(true); pool.shutdown(); pool.awaitTermination(Long.MAX_VALUE, TimeUnit.SECONDS); } } ----------------------------------------------------- ==> java -esa -ea STPE threw! java.lang.Error at STPE$1.run(STPE.java:29) at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:441) at java.util.concurrent.FutureTask$Sync.innerRun(FutureTask.java:303) at java.util.concurrent.FutureTask.run(FutureTask.java:138) at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.access$301(ScheduledThreadPoolExecutor.java:98) at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:207) at java.util.concurrent.ThreadPoolExecutor$Worker.runTask(ThreadPoolExecutor.java:885) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:907) at java.lang.Thread.run(Thread.java:619) java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask@186d4c1 null ----------------------- cancelled! java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask@b89838 null -----------------------
14-08-2006

EVALUATION I think ScheduledThreadPoolExecutor is working as designed. Perhaps it could be made clearer that if you are interested in exceptions or returned values, then submit should be used instead of execute. Alternatively, afterExecute can be used. ScheduledThreadPoolExecutor and ThreadPoolExecutor are difficult to understand, for users and maintainers.
10-08-2006