Re: RFR: 8318839: Update test thread factory to catch all exceptions [v2]
On Fri, 3 Nov 2023 03:44:31 GMT, Leonid Mesnik 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. > > Leonid Mesnik has updated the pull request incrementally with one additional > commit since the last revision: > > Replaced System.exit() with exception. ok, seems there is no good way to process exceptions here. - PR Comment: https://git.openjdk.org/jdk/pull/16369#issuecomment-1792774893
Re: RFR: 8318839: Update test thread factory to catch all exceptions [v2]
On Fri, 3 Nov 2023 05:43:38 GMT, David Holmes wrote: > I don't understand what this is trying to do. If any virtual thread has an > uncaught exception then other virtual threads will throw it wrapped in a > RuntimeException. But the first virtual thread that completes (normally or by > throwing) will clear the default UEH that you set, so uncaught exceptions > from other virtual threads won't do anything. > > AFAICS setting the UEH for a virtual thread works fine, so why are you not > setting a per-thread handler? My reading is that it sets default UHE so it means that any thread (not just virtual threads) can potentially execute it. There will be tests that set a per thread UHE that doesn't delegate. There will also be platform threads in a jtreg ThreadGroup (a UHE) so the exceptions will be handled there (AFAIK, the jtreg TG/UGE does not delegate to the default UHE). I think the patch is confusing because uncaughtException may be set several times, last one wins. If virtual "main" completes without an exception then it looks at uncaughtException to see if an exception is recorded by another thread. It does wrap/propagate it as a runtime exception and I think the bit we aren't seeing is that this is handled by the real main on a platform thread in the jtreg TG. If the virtual "main" completes with an exception (meaning task throws), then it resets the default UHE and exits with an uncaught handle. As with the runtime exception case, I think we aren't seeing this being handled by real main. So I think it is working but confusing to read. I wonder if it might be better to just put the effort into helping CODETOOLS-7903526 instead of a workaround. - PR Review Comment: https://git.openjdk.org/jdk/pull/16369#discussion_r1381220891
Re: RFR: 8318839: Update test thread factory to catch all exceptions [v2]
On Fri, 3 Nov 2023 03:44:31 GMT, Leonid Mesnik 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. > > Leonid Mesnik has updated the pull request incrementally with one additional > commit since the last revision: > > Replaced System.exit() with exception. Changes requested by dholmes (Reviewer). test/jtreg_test_thread_factory/src/share/classes/Virtual.java line 76: > 74: uncaughtException = null; > 75: Thread.setDefaultUncaughtExceptionHandler(null); > 76: } I don't understand what this is trying to do. If any virtual thread has an uncaught exception then other virtual threads will throw it wrapped in a RuntimeException. But the first virtual thread that completes (normally or by throwing) will clear the default UEH that you set, so uncaught exceptions from other virtual threads won't do anything. AFAICS setting the UEH for a virtual thread works fine, so why are you not setting a per-thread handler? - PR Review: https://git.openjdk.org/jdk/pull/16369#pullrequestreview-1711859772 PR Review Comment: https://git.openjdk.org/jdk/pull/16369#discussion_r1381179648
Re: RFR: 8318839: Update test thread factory to catch all exceptions [v2]
On Fri, 3 Nov 2023 03:44:31 GMT, Leonid Mesnik 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. > > Leonid Mesnik has updated the pull request incrementally with one additional > commit since the last revision: > > Replaced System.exit() with exception. I updated the test thread factory to check if there are unhandled exceptions after the test is completed. It now works similarly to what I plan to fix in jtreg. So the purpose of this fix is to catch all exceptions with factory enabled and also to "pre-test" jtreg fix in some cases. - PR Comment: https://git.openjdk.org/jdk/pull/16369#issuecomment-1791859353
Re: RFR: 8318839: Update test thread factory to catch all exceptions [v2]
> 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. Leonid Mesnik has updated the pull request incrementally with one additional commit since the last revision: Replaced System.exit() with exception. - Changes: - all: https://git.openjdk.org/jdk/pull/16369/files - new: https://git.openjdk.org/jdk/pull/16369/files/8fbb2798..b0878f35 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16369=01 - incr: https://webrevs.openjdk.org/?repo=jdk=16369=00-01 Stats: 48 lines in 1 file changed: 37 ins; 10 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16369.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16369/head:pull/16369 PR: https://git.openjdk.org/jdk/pull/16369
