On Sun, 29 Oct 2023 14:10:32 GMT, Jaikiran Pai <j...@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
