RFR: 8290732: JNI DestroyJavaVM can start shutdown when one non-daemon thread remains

[Thomas Stuefe]

On Tue, 9 Aug 2022 06:10:40 GMT, David Holmes <dholmes at openjdk.org> 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