RFR: 8318839: Update test thread factory to catch all exceptions

[David Holmes]

On Sun, 29 Oct 2023 14:10:32 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> The jtreg starts the main thread in a separate ThreadGroup and checks unhandled exceptions for this group. However, it doesn't catch all unhandled exceptions. There is a jtreg issue for this https://bugs.openjdk.org/browse/CODETOOLS-7903526.
>> Catching such issues for virtual threads is important because they are not included in any groups. So this fix implements the handler for the test thread factory. 
>> 
>> A few tests start failing.
>> 
>> The test
>> serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorVMEventsTest.java
>> has testcases for platform and virtual threads. So, there is there's no need to run it with the thread factory.
>> 
>> The test
>> java/lang/Thread/virtual/ThreadAPI.java
>> tests UncaughtExceptionHandler and virtual threads. No need to run it with a thread factory.
>> 
>> Test 
>> test/jdk/java/util/concurrent/tck/ThreadTest.java is updated to not check the default empty handler.
>> 
>> Probably, we need some common approach about dealing with the UncaughtExceptionHandler in jtreg.
>
> Hello Leonid, in order to understand what exactly we are trying to solve here, I ran a few tests to see how things work without the changes being proposed in this PR. Here's my findings.
> 
> A bit of background first. When jtreg runs, either in agent mode or othervm mode, it creates a specific thread within which the actual test code runs. In either of these modes, it uses a custom jtreg specific `ThreadGroup` instance for this thread which is running the test code. This instance of `ThreadGroup` has specific overridden implementation of the `public void uncaughtException(Thread t, Throwable e)` API which keeps track of uncaught exception that might have been thrown by any threads that could have been spawned by the test. After `start()`ing the thread which runs the test code, the jtreg framework then waits for that thread to complete and once completed (either exceptionally or normally), jtreg framework then queries a state on the custom `ThreadGroup` instance to see if any uncaught exception(s) were received during the lifetime of this thread which ran that test. If it finds any, then it marks the test as failed and reports such a failure appropriately in the test re
 port. As noted, this applies for both the agent mode and other vm mode. Some important aspects of this implementation is that:
> 
> - The custom `ThreadGroup` which has the overridden implementation of the `uncaughtException(Thread t, Throwable e)` method doesn't interfere with the JVM level default exception handler.
> 
> - After the thread which ran the test code completes, the decision on whether to fail or pass a test is taken by checking the custom `ThreadGroup`'s state. Once this decision is done, the decision is immediately reported in relevant ways and the test status is marked (i.e. finalized) at this point.
> 
> - If this was running in agent vm mode, the agent vm mode continues to operate and isn't terminated and thus is available for subsequent tests. This point I think is important to remember for reasons that will be noted later in this comment.
> 
> Now coming to the part where in https://bugs.openjdk.org/browse/JDK-8303703 we introduced a way where jtreg instead of creating a platform thread (backed by its custom implementation of the `ThreadGroup`) to run the test code, now checks the presence of a `ThreadFactory` and if present, lets it create a `Thread` which runs this test code. In the JDK repo, we have an implementation of a `ThreadFactory` which creates a virtual thread instead of platform...

thanks for that detailed analysis @jaikiran ! I'm very uncomfortable with what is proposed in this PR.

I would hope that when jtreg uses the virtual thread factory then we could provide a wrapper that will execute the real task in a try/catch and allow any uncaught exceptions to be processed in the way jtreg needs them to be.

-------------

PR Comment: https://git.openjdk.org/jdk/pull/16369#issuecomment-1784672682