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

[Jaikiran Pai]

On Wed, 25 Oct 2023 21:08:01 GMT, Leonid Mesnik <lmesnik 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, looking at the changes in this PR, what's being proposed is that when jtreg lauches tests through a virtual thread, then this wrapping code will set a JVM level `UncaughtExceptionHandler` by calling `Thread.setDefaultUncaughtExceptionHandler(...)`. The implementation of this `UncaughtExceptionHandler` calls `System.exit(1)`. Wouldn't that kill the test VM? I think that would then impact everything else including jtreg report generation and such for the test, isn't it? 

I had a look at https://bugs.openjdk.org/browse/JDK-8318839 but it doesn't have enough details to help understand what currently happens when a test launched through a virtual thread from jtreg throws an uncaught exception. How/what gets reported for that test execution?

test/jdk/java/util/concurrent/tck/ThreadTest.java line 79:

> 77:      */
> 78:     public void testGetAndSetDefaultUncaughtExceptionHandler() {
> 79:         assertNull(Thread.getDefaultUncaughtExceptionHandler());

I think this is a very broad change in this test case and either shouldn't be done or should be done conditionally (I can't think of the right condition now because I haven't fully grasped the context of this PR).

test/jtreg_test_thread_factory/src/share/classes/Virtual.java line 37:

> 35:             // The virtual threads don't belong to any group and need global handler.
> 36:             Thread.setDefaultUncaughtExceptionHandler((t, e) -> {
> 37:                     if (e instanceof ThreadDeath) {

`ThreadDeath` has been deprecated for removal since Java 20, so this should no longer be needed.

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

PR Comment: https://git.openjdk.org/jdk/pull/16369#issuecomment-1780470987
PR Review Comment: https://git.openjdk.org/jdk/pull/16369#discussion_r1372631138
PR Review Comment: https://git.openjdk.org/jdk/pull/16369#discussion_r1372630017