On Tue, 9 Aug 2022 06:10:40 GMT, David Holmes <[email protected]> wrote:
> Please review this fix for a problem discovered by @stuart-marks in the > course of examining the VM shutdown behaviour. The VM code assumed that only > unattached threads called JNI's DestroyJavaVM and so they were always > attached as non-daemon threads. But it is perfectly valid to call > DestroyJavaVM from an already attached thread, which could be a daemon. The > fix simply checks whether the caller is a daemon or not and adjusts the > expected count of active threads to see. There is also an adjustment to the > thread termination logic to also notify at the right time. > > Thanks to @stuart-marks for the reproducer in JBS - the longest part of this > by many hours was converting the test over the jtreg. :) > > Testing: > - the new test on all core platforms > - tiers 1-3 > Thanks. Hi David, that was interesting. I always thought you have to detach before destroying the VM. I looked into https://docs.oracle.com/javase/7/docs/technotes/guides/jni/spec/invocation.html and it says: "Detaching from the VM: A native thread attached to the VM must call DetachCurrentThread() to detach itself before exiting." Cheers, Thomas test/hotspot/jtreg/runtime/jni/daemonDestroy/Main.java line 36: > 34: static volatile Thread t1; > 35: > 36: // Note: not normal main(String[] args) Nit: why is it important that this not be a normal main? Maybe just rename it to something very different? test/hotspot/jtreg/runtime/jni/daemonDestroy/TestDaemonDestroy.java line 36: > 34: * @run main/native TestDaemonDestroy > 35: * @run main/native TestDaemonDestroy daemon > 36: */ If you do two @test sections, you can give them speaking names(e.g. @test id=demon) and they can run in parallel. test/hotspot/jtreg/runtime/jni/daemonDestroy/TestDaemonDestroy.java line 59: > 57: System.out.println("Launcher = " + launcher + > 58: (Files.exists(launcher) ? " (exists)" : " > (missing)")); > 59: Nit: better arg handling? - If you want to just have one char arg, pass 'D' and 'N' for demon/nondemon mode instead of "1" and "". - Instead of using args.length, maybe actually parse the input argument, or specify D and N there too and pass it on. test/hotspot/jtreg/runtime/jni/daemonDestroy/TestDaemonDestroy.java line 72: > 70: oa.shouldHaveExitValue(0); > 71: oa.shouldNotContain("Error: T1 isAlive"); > 72: oa.reportDiagnosticSummary(); Should we not add an `oa.shouldContain("T1 finished")` ? ------------- PR: https://git.openjdk.org/jdk/pull/9803
