On Sat, 6 Aug 2022 00:42:23 GMT, Stuart Marks <sma...@openjdk.org> wrote:
>> Initial edits to addShutdownHook from Alex. >> >> See [JDK-8290036](https://bugs.openjdk.org/browse/JDK-8290036). > > Stuart Marks has updated the pull request incrementally with one additional > commit since the last revision: > > More edits from Alex's suggestions. Hi Stuart, Looking good overall. A few minor comments and one more substantive one regarding `DestroyJavaVM`. Thanks, David src/java.base/share/classes/java/lang/Runtime.java line 55: > 53: * to one of several events: > 54: * <ol> > 55: * <li>when the number of {@linkplain Thread#isAlive() live} non-daemon > threads drops to zero Can we somehow forward-reference the implNote about DestroyJavaVM here? src/java.base/share/classes/java/lang/Runtime.java line 66: > 64: * {@linkplain Thread#start started} in some unspecified order. They run > concurrently > 65: * with any daemon or non-daemon threads that were {@linkplain > Thread#isAlive() alive} > 66: * at the beginning of the shutdown sequence. or which subsequently get started as covered later. So perhaps a simple reordering here: > <p>At the beginning of the shutdown sequence, the registered shutdown hooks > are {@linkplain Thread#start started} in some unspecified order. > <p>After the shutdown sequence has begun, registration and de-registration of > shutdown hooks with {@link #addShutdownHook addShutdownHook} and {@link > #removeShutdownHook removeShutdownHook} is prohibited. However, creating and > starting new threads is permitted. > <p> The shutdown hooks run concurrently with any daemon or non-daemon threads > that were {@linkplain Thread#isAlive() alive} at the beginning of the > shutdown sequence; or which are subsequently started. src/java.base/share/classes/java/lang/Runtime.java line 85: > 83: * effect on the shutdown sequence is unspecified. > 84: * > 85: * <h2><a id="termination">Virtual Machine Termination</a></h2> Should this say "Java Virtual Machine Termination" for consistency. src/java.base/share/classes/java/lang/Runtime.java line 99: > 97: * <a > href="{@docRoot}/../specs/jni/invocation.html#destroyjavavm">DestroyJavaVM</a>. > 98: * This function is responsible for initiating the shutdown sequence when > the number of running > 99: * ({@linkplain Thread#isAlive() live}) non-daemon threads first drops to > zero. When the shutdown I think we need to align this more with the proposed text of JLS 12.8. It needs to be clearer that `DestroyJavaVM` must be used in conjunction with `CreateJavaVM` to get the desired termination behaviour. Otherwise, the proposed text allows for the possibility that the JVM will always terminate when the last non-daemon thread terminates, even if `DestroyJavaVM` has not been called. It needs to be clear that if the JVM is started by the JNI Invocation API, then it must also be terminated by the JNI invocationAPI to get the required behaviour. ------------- Changes requested by dholmes (Reviewer). PR: https://git.openjdk.org/jdk/pull/9437
