RFR: JDK-8240148: Uncaught exceptions from ScheduledThreadPoolExecutor.execute aren't reported

[David Holmes]

Hi Jaikiran,

On 24/06/2020 11:44 pm, Jaikiran Pai wrote:
> Could I please get a review and a sponsor for a fix for
> https://bugs.openjdk.java.net/browse/JDK-8240148?

I missed the filing of this bug. :(

> The patch is available as a webrev at
> https://cr.openjdk.java.net/~jpai/webrev/8240148/1/
> 
> The patch updates the ScheduledThreadPoolExecutor.execute(Runnable) to
> wrap the user passed Runnable into a custom one which then catches any
> Throwable and reports it through the Thread's uncaught exception
> handler. After having reviewed the existing code in that class, this was
> the easiest (and perhaps the only feasible?) and cleanest way that I
> could think of, to fix this issue.

I do not agree with the solution or even the formulation of the issue.

This is something that has been raised and discussed a number of times 
since Java 5. See for example:

http://cs.oswego.edu/pipermail/concurrency-interest/2011-April/007865.html

The UncaughtExceptionHandler is, as the name clearly indicates, for 
execution in response to uncaught exceptions and is the last act of a 
dying thread. You are invoking it for a Runnable just because you happen 
to like to what it does by default for threads - and you don't even know 
what it will do as it may have been customised. It is an implementation 
detail with TPE and STPE whether exceptions from tasks lead to 
termination of the worker thread and thus result in execution of the 
UEH, or whether they catch them and continue. For TPE they can; but for 
STPE they never will as the task is wrapped in a Future.

The advice has always been that any Runnable submitted as a task to an 
ExecutorService should take responsibility for dealing with any 
exceptions it encounters.

David
-----

> 
> A few things about this change:
> 
> 1. I decided to use an anonymous class for the Runnable, in the
> execute() method instead of a lambda because I was unsure if this part
> of the JDK code is allowed to use a lambda. In the past I have seen
> discussions where it was recommended not use lambda in certain parts of
> the JDK either because of performance(?) reason or because "this part of
> the code runs too early to be using a lambda" reason. I personally
> prefer an anonymous class anyway, but if others feel using a lambda here
> is relevant, then let me know.
> 
> 2. Once the exception is reported using the UncaughtExceptionHandler, I
> just return. Initially, I had considered throwing back the original
> exception/throwable after reporting it via the UncaughtExceptionHandler,
> but having looked at few others places within the JDK which do not throw
> back the original exception, I decided to follow the same semantics.
> 
> 3. Now that the passed Runnable is wrapped and submitted for execution,
> I considered whether this would break any (unwritten) contract between
> the ThreadPoolExecutor and the ThreadFactory#newThread(Runnable)
> implementations. If any (JDK internal or user application specific)
> ThreadFactory#newThread method was expecting the passed Runnable to be
> of the same type or the same instance as that was submitted to the
> ScheduledThreadPoolExecutor#execute(Runnable), then this change would
> break such ThreadFactory#newThread implementations. I looked deeper in
> the ThreadPoolExecutor code and from what I could see, this isn't a
> practical concern, since even without this change, The
> ThreadPoolExecutor in its private Worker class already sends a
> "delegate" Runnable (an instance of the
> ThreadPoolExecutor$Worker class) to the ThreadFactory's newThread method
> and not the original Runnable that was submitted to the
> execute(Runnable) method of the executor. So it doesn't look like
> there's any written/unwritten contract that the ThreadFactory is
> expected to receive the same Runnable instance that was passed to the
> execute method of the executor.
> 
> 4. The ScheduledThreadPoolExecutor has a different license than some of
> the other files that I am used to, within the JDK code. So I haven't
> edited it for any of the usual copyright year updates.
> 
> 5. The patch contains a new (testng based) test case which reproduces
> this issue and verifies the fix.
> 
> -Jaikiran
> 
>