Re: RFR: 8290036: Define and specify Runtime shutdown sequence [v4]
> 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. - Changes: - all: https://git.openjdk.org/jdk/pull/9437/files - new: https://git.openjdk.org/jdk/pull/9437/files/5a16ea30..d348cca4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=9437&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=9437&range=02-03 Stats: 11 lines in 1 file changed: 1 ins; 2 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/9437.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9437/head:pull/9437 PR: https://git.openjdk.org/jdk/pull/9437
Re: RFR: 8290036: Define and specify Runtime shutdown sequence [v4]
On Sat, 6 Aug 2022 00:42:23 GMT, Stuart Marks 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.
src/java.base/share/classes/java/lang/System.java line 1888:
> 1886: * Initiates the shutdown
> sequence of the Java Virtual
> 1887: * Machine. This method always blocks indefinitely. The
> 1888: * argument serves as a status code; by convention, a nonzero status
Would you remind reflowing this one too as the latest change makes mixes the
line lengths again.
src/java.base/share/classes/java/lang/Thread.java line 73:
> 71: * or if its {@code run} method completes abruptly and the appropriate
> 72: * {@linkplain Thread.UncaughtExceptionHandler uncaught exception
> handler} completes
> 73: * normally or abruptly. With no code left to run, the thread has
> completed execution.
I'm in two minds about introducing the UHE in paragraph 3 but probably okay as
you kinda have to mention it when defining termination. There's a lot more to
this story when you have agents in the picture but I'll not go there. I wonder
if we can do something with the last sentence that includes the word
"terminated". We use "terminated" in several places and it would be cleared if
this sentence were to end with something like "... the thread has completes
execution, and is terminated" (it could link to isAlive or
Thread.State#TERMINATED).
Given that the join method is introduced as the method to wait for a thread to
terminate then it could be part of this paragraph rather than a single sentence
paragraph.
src/java.base/share/classes/java/lang/Thread.java line 104:
> 102: * The shutdown sequence begins when
> all started
> 103: * non-daemon threads have terminated. Unstarted non-daemon threads do
> not prevent
> 104: * the shutdown sequence from commencing. Invoking the {@linkplain
> Runtime#exit(int)}
I think this is the only usage of "commencing", everywhere else uses
"beginning" or "begin the shutdown sequence".
-
PR: https://git.openjdk.org/jdk/pull/9437
Re: RFR: 8290036: Define and specify Runtime shutdown sequence [v4]
On Mon, 8 Aug 2022 02:34:08 GMT, David Holmes wrote:
>> src/java.base/share/classes/java/lang/Thread.java line 104:
>>
>>> 102: * The shutdown sequence begins
>>> when all started
>>> 103: * non-daemon threads have terminated. Unstarted non-daemon threads do
>>> not prevent
>>> 104: * the shutdown sequence from commencing. Invoking the {@linkplain
>>> Runtime#exit(int)}
>>
>> I think this is the only usage of "commencing", everywhere else uses
>> "beginning" or "begin the shutdown sequence".
>
> "initiated" is also used.
I would suggest saying as little as possible here and simply deferring to the
Runtime text ie.:
> The shutdown sequence begins when all
> started
non-daemon threads have terminated.
No need to elaborate, or directly refer to exit().
-
PR: https://git.openjdk.org/jdk/pull/9437
Re: RFR: 8290036: Define and specify Runtime shutdown sequence [v4]
On Sat, 6 Aug 2022 08:11:53 GMT, Alan Bateman wrote:
>> Stuart Marks has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> More edits from Alex's suggestions.
>
> src/java.base/share/classes/java/lang/Thread.java line 73:
>
>> 71: * or if its {@code run} method completes abruptly and the appropriate
>> 72: * {@linkplain Thread.UncaughtExceptionHandler uncaught exception
>> handler} completes
>> 73: * normally or abruptly. With no code left to run, the thread has
>> completed execution.
>
> I'm in two minds about introducing the UHE in paragraph 3 but probably okay
> as you kinda have to mention it when defining termination. There's a lot more
> to this story when you have agents in the picture but I'll not go there. I
> wonder if we can do something with the last sentence that includes the word
> "terminated". We use "terminated" in several places and it would be clearer
> if this sentence were to end with something like "... the thread has
> completes execution, and is terminated" (it could link to isAlive or
> Thread.State#TERMINATED).
>
> Given that the join method is introduced as the method to wait for a thread
> to terminate then it could be part of this paragraph rather than a single
> sentence paragraph.
I would also suggest that the join() sentence simply be the last sentence of
the above paragraph:
> The join method can be be used to wait for a thread to terminate.
> src/java.base/share/classes/java/lang/Thread.java line 104:
>
>> 102: * The shutdown sequence begins
>> when all started
>> 103: * non-daemon threads have terminated. Unstarted non-daemon threads do
>> not prevent
>> 104: * the shutdown sequence from commencing. Invoking the {@linkplain
>> Runtime#exit(int)}
>
> I think this is the only usage of "commencing", everywhere else uses
> "beginning" or "begin the shutdown sequence".
"initiated" is also used.
-
PR: https://git.openjdk.org/jdk/pull/9437
Re: RFR: 8290036: Define and specify Runtime shutdown sequence [v4]
On Sat, 6 Aug 2022 00:42:23 GMT, Stuart Marks 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: *
> 55: * 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:
> At the beginning of the shutdown sequence, the registered shutdown hooks
> are {@linkplain Thread#start started} in some unspecified order.
> 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.
> 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: * Virtual Machine Termination
Should this say "Java Virtual Machine Termination" for consistency.
src/java.base/share/classes/java/lang/Runtime.java line 99:
> 97: * href="{@docRoot}/../specs/jni/invocation.html#destroyjavavm">DestroyJavaVM.
> 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
Re: RFR: 8290036: Define and specify Runtime shutdown sequence [v4]
On Sat, 6 Aug 2022 07:47:04 GMT, Alan Bateman wrote: >> Stuart Marks has updated the pull request incrementally with one additional >> commit since the last revision: >> >> More edits from Alex's suggestions. > > src/java.base/share/classes/java/lang/System.java line 1888: > >> 1886: * Initiates the shutdown >> sequence of the Java Virtual >> 1887: * Machine. This method always blocks indefinitely. The >> 1888: * argument serves as a status code; by convention, a nonzero >> status > > Would you remind reflowing this one too as the latest change means lines > length are inconsistent. Will do. - PR: https://git.openjdk.org/jdk/pull/9437
Re: RFR: 8290036: Define and specify Runtime shutdown sequence [v4]
On Mon, 8 Aug 2022 02:36:45 GMT, David Holmes wrote: >> "initiated" is also used. > > I would suggest saying as little as possible here and simply deferring to the > Runtime text ie.: > >> The shutdown sequence begins when all >> started > non-daemon threads have terminated. > > No need to elaborate, or directly refer to exit(). I changed "commencing" to beginning, and I removed the sentence talking about Runtime::exit. I think it's useful to be explicit about unstarted non-daemon threads. - PR: https://git.openjdk.org/jdk/pull/9437
Re: RFR: 8290036: Define and specify Runtime shutdown sequence [v4]
On Mon, 8 Aug 2022 02:33:13 GMT, David Holmes wrote:
>> src/java.base/share/classes/java/lang/Thread.java line 73:
>>
>>> 71: * or if its {@code run} method completes abruptly and the appropriate
>>> 72: * {@linkplain Thread.UncaughtExceptionHandler uncaught exception
>>> handler} completes
>>> 73: * normally or abruptly. With no code left to run, the thread has
>>> completed execution.
>>
>> I'm in two minds about introducing the UHE in paragraph 3 but probably okay
>> as you kinda have to mention it when defining termination. There's a lot
>> more to this story when you have agents in the picture but I'll not go
>> there. I wonder if we can do something with the last sentence that includes
>> the word "terminated". We use "terminated" in several places and it would be
>> clearer if this sentence were to end with something like "... the thread has
>> completes execution, and is terminated" (it could link to isAlive or
>> Thread.State#TERMINATED).
>>
>> Given that the join method is introduced as the method to wait for a thread
>> to terminate then it could be part of this paragraph rather than a single
>> sentence paragraph.
>
> I would also suggest that the join() sentence simply be the last sentence of
> the above paragraph:
>
>> The join method can be be used to wait for a thread to terminate.
The whole paragraph is about thread termination, so I don't think we need to
say "terminate" again in the last sentence. We could mention isAlive or the
TERMINATED state, but that brings in Thread.State which which require a
discussion of the thread's life cycle.
The "join" sentence was kind of tacked on to the end of a paragraph in the
original text; Alex suggested that I separate it into its own paragraph when we
were working on the "termination" paragraph. Alex doesn't mind single-sentence
paragraphs, which isn't my preference. But tacking the "join" sentence onto the
end of the "termination" paragraph actually doesn't make any sense. Termination
is something that happens to _this_ thread. The join method is called from
_another_ thread, and this implicit shift of viewpoint would weaken the
paragraph.
Frankly I think there needs to be a separate editorial pass over the Thread
docs. I know they were just rewritten in JDK 19, but there are some odd things
about it. The Thread.State life cycle isn't explained and isn't referenced by
methods such as start() and isAlive(). Also, various mentions of uncaught
exception handling talk about a thread's abrupt termination. There isn't any
such thing; a _method_ can complete normally or abruptly, and when a thread's
run() method completes either way, the thread simply terminates.
-
PR: https://git.openjdk.org/jdk/pull/9437
Re: RFR: 8290036: Define and specify Runtime shutdown sequence [v4]
On Mon, 8 Aug 2022 01:55:08 GMT, David Holmes wrote:
>> Stuart Marks has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> More edits from Alex's suggestions.
>
> 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:
>
>> At the beginning of the shutdown sequence, the registered shutdown hooks
>> are {@linkplain Thread#start started} in some unspecified order.
>
>> 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.
>
>> 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.
Alex and I have gone over this about three times. Your suggestion is shorter,
but it requires a closer read to see all the cases. In particular, it answers
the questions "what happens to currently running threads" and specifically
mentions both daemon and non-daemon threads. Various earlier bits of text
implied incorrectly that non-daemon threads could no longer be running during
the shutdown sequence or even at the end of the shutdown sequence. Being
explicit makes things somewhat redundant but it covers the cases more
thoroughly.
> src/java.base/share/classes/java/lang/Runtime.java line 85:
>
>> 83: * effect on the shutdown sequence is unspecified.
>> 84: *
>> 85: * Virtual Machine Termination
>
> Should this say "Java Virtual Machine Termination" for consistency.
will fix
-
PR: https://git.openjdk.org/jdk/pull/9437
Re: RFR: 8290036: Define and specify Runtime shutdown sequence [v4]
On Mon, 8 Aug 2022 02:06:18 GMT, David Holmes wrote:
>> Stuart Marks has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> More edits from Alex's suggestions.
>
> src/java.base/share/classes/java/lang/Runtime.java line 55:
>
>> 53: * to one of several events:
>> 54: *
>> 55: * when the number of {@linkplain Thread#isAlive() live} non-daemon
>> threads drops to zero
>
> Can we somehow forward-reference the implNote about DestroyJavaVM here?
added
> src/java.base/share/classes/java/lang/Runtime.java line 99:
>
>> 97: * > href="{@docRoot}/../specs/jni/invocation.html#destroyjavavm">DestroyJavaVM.
>> 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.
OK, yes I accept the general premise of your comment. Indeed we need to make
sure this section is well aligned with JLS 12.8 (as well as the JNI spec and
the JVMS, though I think those can be a bit more distant). I see that Alex has
updated JLS 12.8 more recently than your comment, so I'll hold off editing this
section for now and check with him to make sure he's done editing before I
proceed. We might need some additional discussion as well. So, more to come.
-
PR: https://git.openjdk.org/jdk/pull/9437
Re: RFR: 8290036: Define and specify Runtime shutdown sequence [v4]
On Mon, 8 Aug 2022 22:34:00 GMT, Stuart Marks wrote: >> I would also suggest that the join() sentence simply be the last sentence of >> the above paragraph: >> >>> The join method can be be used to wait for a thread to terminate. > > The whole paragraph is about thread termination, so I don't think we need to > say "terminate" again in the last sentence. We could mention isAlive or the > TERMINATED state, but that brings in Thread.State which which require a > discussion of the thread's life cycle. > > The "join" sentence was kind of tacked on to the end of a paragraph in the > original text; Alex suggested that I separate it into its own paragraph when > we were working on the "termination" paragraph. Alex doesn't mind > single-sentence paragraphs, which isn't my preference. But tacking the "join" > sentence onto the end of the "termination" paragraph actually doesn't make > any sense. Termination is something that happens to _this_ thread. The join > method is called from _another_ thread, and this implicit shift of viewpoint > would weaken the paragraph. > > Frankly I think there needs to be a separate editorial pass over the Thread > docs. I know they were just rewritten in JDK 19, but there are some odd > things about it. The Thread.State life cycle isn't explained and isn't > referenced by methods such as start() and isAlive(). Also, various mentions > of uncaught exception handling talk about a thread's abrupt termination. > There isn't any such thing; a _method_ can complete normally or abruptly, and > when a thread's run() method completes either way, the thread simply > terminates. Thread.State was added (in Java 5) for monitoring and management purposes, it's not an API that most libraries or programs would use. It could be introduced in the class description and referenced from other APIs that change/test the state but it would encourage use beyond the original intent. - PR: https://git.openjdk.org/jdk/pull/9437
Re: RFR: 8290036: Define and specify Runtime shutdown sequence [v4]
On Tue, 9 Aug 2022 05:51:17 GMT, Alan Bateman wrote: >> The whole paragraph is about thread termination, so I don't think we need to >> say "terminate" again in the last sentence. We could mention isAlive or the >> TERMINATED state, but that brings in Thread.State which which require a >> discussion of the thread's life cycle. >> >> The "join" sentence was kind of tacked on to the end of a paragraph in the >> original text; Alex suggested that I separate it into its own paragraph when >> we were working on the "termination" paragraph. Alex doesn't mind >> single-sentence paragraphs, which isn't my preference. But tacking the >> "join" sentence onto the end of the "termination" paragraph actually doesn't >> make any sense. Termination is something that happens to _this_ thread. The >> join method is called from _another_ thread, and this implicit shift of >> viewpoint would weaken the paragraph. >> >> Frankly I think there needs to be a separate editorial pass over the Thread >> docs. I know they were just rewritten in JDK 19, but there are some odd >> things about it. The Thread.State life cycle isn't explained and isn't >> referenced by methods such as start() and isAlive(). Also, various mentions >> of uncaught exception handling talk about a thread's abrupt termination. >> There isn't any such thing; a _method_ can complete normally or abruptly, >> and when a thread's run() method completes either way, the thread simply >> terminates. > > Thread.State was added (in Java 5) for monitoring and management purposes, > it's not an API that most libraries or programs would use. It could be > introduced in the class description and referenced from other APIs that > change/test the state but it would encourage use beyond the original intent. What you have is okay although I would have preferred if the sentence on the join method was in the previous paragraph rather as it fits with termination. - PR: https://git.openjdk.org/jdk/pull/9437
Re: RFR: 8290036: Define and specify Runtime shutdown sequence [v4]
On Mon, 8 Aug 2022 02:27:15 GMT, David Holmes wrote:
>> Stuart Marks has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> More edits from Alex's suggestions.
>
> src/java.base/share/classes/java/lang/Runtime.java line 99:
>
>> 97: * > href="{@docRoot}/../specs/jni/invocation.html#destroyjavavm">DestroyJavaVM.
>> 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.
@dholmes-ora OK I've updated the implementation note to try to make it quite
clear that the shutdown-at-zero-non-daemon-threads behavior is tied to
`DestroyJavaVM`. (This is good; I didn't know this before I embarked on this
exercise of updating these specs!) I'm not entirely sure of the state of JLS
12.8, but if I know Alex :-) it's unlikely to change much, and I think this
section is consistent with it.
-
PR: https://git.openjdk.org/jdk/pull/9437
Re: RFR: 8290036: Define and specify Runtime shutdown sequence [v4]
On Tue, 9 Aug 2022 13:56:44 GMT, Alan Bateman wrote: >> Thread.State was added (in Java 5) for monitoring and management purposes, >> it's not an API that most libraries or programs would use. It could be >> introduced in the class description and referenced from other APIs that >> change/test the state but it would encourage use beyond the original intent. > > What you have is okay although I would have preferred if the sentence on the > join method was in the previous paragraph rather as it fits with termination. Plus one on the join() bit. You could also argue for mentioning isAlive() here too. - PR: https://git.openjdk.org/jdk/pull/9437
Re: RFR: 8290036: Define and specify Runtime shutdown sequence [v4]
On Thu, 11 Aug 2022 07:23:28 GMT, David Holmes wrote: >> What you have is okay although I would have preferred if the sentence on the >> join method was in the previous paragraph rather as it fits with termination. > > Plus one on the join() bit. You could also argue for mentioning isAlive() > here too. Merged `join` sentence into previous paragraph per David's earlier suggestion. I didn't mention `isAlive` though, no need to complicate this further. - PR: https://git.openjdk.org/jdk/pull/9437
