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.forums2...@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 > >
