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

[Martin Buchholz]

Thanks for your work on this.

Invoking the UEH machinery directly is extremely unusual and would
never have occurred to me (outside of a test).

In a loom-directed world, any attempt to directly manipulate the
current thread is likely to lead to trouble.

This code is primarily maintained separately at Doug's site
http://gee.cs.oswego.edu/dl/concurrency-interest/
I've been handling most of the integration work.

On Wed, Jun 24, 2020 at 6:51 AM Jaikiran Pai <jai.forums2013 at gmail.com> wrote:
>
> Could I please get a review and a sponsor for a fix for
> https://bugs.openjdk.java.net/browse/JDK-8240148?
>
> 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.
>
> 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
>
>