Re: RFR: 8288984: Simplification in Shutdown.exit [v2]
On Tue, 5 Jul 2022 04:51:50 GMT, Ryan Ernst wrote: >> YAO (Yet Another Opinion)! But I think we're actually converging. >> >> David's wording: >>>Invocations of this method are serialized such that only one invocation will >>>actually proceed with the shutdown sequence and terminate the VM with the >>>given status code. >> >> ... gives the impression that an invocation of this method WILL terminate >> the VM with the given status code - which is not actually true, given the >> potential for signals. This is already alluded to in >> `Runtime::addShutdownHook`. I agree that this could be resolved separately, >> but we should _allow_ for signals (even if we don't explicitly mention them). >> >> The original wording: >>> If this method is invoked after all shutdown hooks have already been run >>> ... >> >> ... seems quite clever (and allows for signals). Maybe we could keep things >> super simple: >>> If this method is invoked after shutdown hooks have already been started, >>> it will block indefinitely. If this method is invoked from a shutdown hook >>> the system will deadlock. > > I like the new suggested wording, but I would slightly tweak it. As Alan > mentioned in another comment the first paragraph makes clear the method never > returns normally. How does the following sound? > >> If this method is invoked after shutdown hooks have already been started, >> the supplied status code will be ignored. If this method is invoked from a >> shutdown hook the system will deadlock. > gives the impression that an invocation of this method WILL terminate the VM > with the given status code - which is not actually true, given the potential > for signals. The subtle distinction being calling Runtime.exit versus Shutdown.exit. The actual serialization takes places in Shutdown.exit and serializes with signals too. But discussion of signals has no place here - I'm not even sure we document those ways of terminating the VM anywhere? - PR: https://git.openjdk.org/jdk/pull/9351
Re: RFR: 8288984: Simplification in Shutdown.exit [v2]
On Mon, 4 Jul 2022 16:47:28 GMT, Chris Hegarty wrote: >> I think the wording in the latest commit (9d972b) is problematic. One reason >> is that you've changed it to "will run shutdown hooks" but it doesn't run >> the hooks, it starts them, and so conflicts with the previous paragraph. I >> also think "That invocation may be initiated via platform specific signal >> handlers" is confusing here as there is no notion of "signal handler" to >> reference. There may be scope for text elsewhere, maybe with an implNote for >> signals such as HUP, but I don't think this is the PR for this. So I think >> it better to try the wording that David suggested and see if it needs any >> improvement. > > YAO (Yet Another Opinion)! But I think we're actually converging. > > David's wording: >>Invocations of this method are serialized such that only one invocation will >>actually proceed with the shutdown sequence and terminate the VM with the >>given status code. > > ... gives the impression that an invocation of this method WILL terminate the > VM with the given status code - which is not actually true, given the > potential for signals. This is already alluded to in > `Runtime::addShutdownHook`. I agree that this could be resolved separately, > but we should _allow_ for signals (even if we don't explicitly mention them). > > The original wording: >> If this method is invoked after all shutdown hooks have already been run ... > > ... seems quite clever (and allows for signals). Maybe we could keep things > super simple: >> If this method is invoked after shutdown hooks have already been started, it >> will block indefinitely. If this method is invoked from a shutdown hook the >> system will deadlock. I like the new suggested wording, but I would slightly tweak it. As Alan mentioned in another comment the first paragraph makes clear the method never returns normally. How does the following sound? > If this method is invoked after shutdown hooks have already been started, the > supplied status code will be ignored. If this method is invoked from a > shutdown hook the system will deadlock. - PR: https://git.openjdk.org/jdk/pull/9351
Re: RFR: 8288984: Simplification in Shutdown.exit [v2]
On Tue, 5 Jul 2022 07:23:49 GMT, David Holmes wrote:
>> I like the new suggested wording, but I would slightly tweak it. As Alan
>> mentioned in another comment the first paragraph makes clear the method
>> never returns normally. How does the following sound?
>>
>>> If this method is invoked after shutdown hooks have already been started,
>>> the supplied status code will be ignored. If this method is invoked from a
>>> shutdown hook the system will deadlock.
>
>> gives the impression that an invocation of this method WILL terminate the VM
>> with the given status code - which is not actually true, given the potential
>> for signals.
>
> The subtle distinction being calling Runtime.exit versus Shutdown.exit. The
> actual serialization takes places in Shutdown.exit and serializes with
> signals too. But discussion of signals has no place here - I'm not even sure
> we document those ways of terminating the VM anywhere?
@dholmes-ora Agreed - this paragraph should not explicitly mention signals or
any _other_ means of termination.
There is some shutdown hook specific verbiage relating to termination in
`Runtime::addShutdownHook` :
In rare circumstances the virtual machine may abort, that is, stop running
without shutting
down cleanly. This occurs when the virtual machine is terminated externally,
for example
with the SIGKILL signal on Unix or the TerminateProcess call on Microsoft
Windows. The
virtual machine may also abort if a native method goes awry by, for example,
corrupting
internal data structures or attempting to access nonexistent memory. If the
virtual machine
aborts then no guarantee can be made about whether or not any shutdown hooks
will be run.
... but we do not need to go there, and it's not really enough to leverage
anyway.
>The subtle distinction being calling Runtime.exit versus Shutdown.exit. The
>actual serialization takes places in Shutdown.exit and serializes with signals
>too.
Exactly. On further reading of the proposed "Invocations of this method are
serialized such that only one invocation will actually proceed with the
shutdown sequence and terminate the VM with the given status code.", I think
that it is fine (given its position and context in Runtime::exit). My reason
for suggesting an alternative ("If this method is invoked after shutdown hooks
have already been started, it will block indefinitely") was mainly to try to
find something a little simpler while retaining correctness. Given the
discussion so far, either is fine with me.
-
PR: https://git.openjdk.org/jdk/pull/9351
Re: RFR: 8288984: Simplification in Shutdown.exit [v2]
On Mon, 4 Jul 2022 16:31:22 GMT, Alan Bateman wrote: >> I reworded with the suggested text in mind. I also added another sentence >> referencing native signal handlers, which may also initiate shutdown. I >> think this clarification is important so that it is clear the status code of >> all invocations from Java may still be ignored. See >> [2253259](https://github.com/openjdk/jdk/pull/9351/commits/2253259c82b13e7b2186429a80cc42497235b035). > > I think the wording in the latest commit (9d972b) is problematic. One reason > is that you've changed it to "will run shutdown hooks" but it doesn't run the > hooks, it starts them, and so conflicts with the previous paragraph. I also > think "That invocation may be initiated via platform specific signal > handlers" is confusing here as there is no notion of "signal handler" to > reference. There may be scope for text elsewhere, maybe with an implNote for > signals such as HUP, but I don't think this is the PR for this. So I think it > better to try the wording that David suggested and see if it needs any > improvement. YAO (Yet Another Opinion)! But I think we're actually converging. David's wording: >Invocations of this method are serialized such that only one invocation will >actually proceed with the shutdown sequence and terminate the VM with the >given status code. ... gives the impression that an invocation of this method WILL terminate the VM with the given status code - which is not actually true, given the potential for signals. This is already alluded to in `Runtime::addShutdownHook`. I agree that this could be resolved separately, but we should _allow_ for it. The original wording: > If this method is invoked after all shutdown hooks have already been run ... ... seems quite clever (and allows for signals). Maybe we could: > If this method is invoked after shutdown hooks have already been started it > will block indefinitely. If this method is invoked from a shutdown hook the > system will deadlock. - PR: https://git.openjdk.org/jdk/pull/9351
Re: RFR: 8288984: Simplification in Shutdown.exit [v2]
On Mon, 4 Jul 2022 16:13:10 GMT, Ryan Ernst wrote: >> David's refinement looks good. The sentence on deadlock is accurate as >> shutdown hooks run on other threads, not the thread calling System.ext. > > I reworded with the suggested text in mind. I also added another sentence > referencing native signal handlers, which may also initiate shutdown. I think > this clarification is important so that it is clear the status code of all > invocations from Java may still be ignored. See > [2253259](https://github.com/openjdk/jdk/pull/9351/commits/2253259c82b13e7b2186429a80cc42497235b035). I think the wording in the latest commit (9d972b) is problematic. One reason is that you've changed it to "will run shutdown hooks" but it doesn't run the hooks, it starts them, and so conflicts with the previous paragraph. I also think "That invocation may be initiated via platform specific signal handlers" is confusing here as there is no notion of "signal handler" to reference. There may be scope for text elsewhere, maybe with an implNote for signals such as HUP, but I don't think this is the PR for this. So I think it better to try the wording that David suggested and see if it needs any improvement. - PR: https://git.openjdk.org/jdk/pull/9351
Re: RFR: 8288984: Simplification in Shutdown.exit [v2]
On Mon, 4 Jul 2022 08:42:02 GMT, Alan Bateman wrote: >> +1 - except for the "deadlock" part (see other comment). I think the old >> paragraph is at least confusing, and perhaps even just wrong. Let's say >> we've run `shutdown` so run all the hooks but not halted. Then someone >> calls `exit(0)`. That seems to suggest the call will block indefinitely, >> which is neither desirable nor what was actually implemented. > > David's refinement looks good. The sentence on deadlock is accurate as > shutdown hooks run on other threads, not the thread calling System.ext. I reworded with the suggested text in mind. I also added another sentence referencing native signal handlers, which may also initiate shutdown. I think this clarification is important so that it is clear the status code of all invocations from Java may still be ignored. See [2253259](https://github.com/openjdk/jdk/pull/9351/commits/2253259c82b13e7b2186429a80cc42497235b035). - PR: https://git.openjdk.org/jdk/pull/9351
Re: RFR: 8288984: Simplification in Shutdown.exit [v2]
On Sat, 2 Jul 2022 21:21:37 GMT, Ryan Ernst wrote: >> This is a followup to simplify Shutdown.exit after the removal of >> finalizers (https://bugs.openjdk.org/browse/JDK-8198250). Once agreement >> on the approach has been reached in this PR, a CSR will be filed to >> propose the spec change to Runtime.exit. > > Ryan Ernst has updated the pull request incrementally with one additional > commit since the last revision: > > better clarify multiple threads behavior > Let's say we've run shutdown so run all the hooks but not halted. Then > someone calls exit(0). That seems to suggest the call will block > indefinitely, which is neither desirable nor what was actually implemented. If the hook threads do not halt then the exiting thread (which holds the lock) blocks forever in the join(). Any other call to exit blocks trying to acquire the lock. That has always been the way this works - if hook threads don't terminate then the VM doesn't (unless someone calls halt() directly). That is one of the things the window you suggested be closed, allowed - a call to exit(non-zero) could force a call to halt(). - PR: https://git.openjdk.org/jdk/pull/9351
Re: RFR: 8288984: Simplification in Shutdown.exit [v2]
On Mon, 4 Jul 2022 08:07:25 GMT, Chris Hegarty wrote:
>> src/java.base/share/classes/java/lang/Runtime.java line 89:
>>
>>> 87: * of the first invocation will be used; the status codes from
>>> other invocations
>>> 88: * will be ignored. If this method is invoked from a shutdown hook
>>> the system
>>> 89: * will deadlock.
>>
>> Is "deadlock" accurate? I thought Java monitors were reentrant, with the
>> result that a shutdown hook calling `exit` would lead to a recursive
>> `runHooks` which would run all the hooks, including *rerunning* hooks before
>> the one that called exit (which could be bad), and then rerunning the hook
>> that called exit (possibly leading to infinite recursion), then running
>> later hooks, then returning to rerun those later hooks. This situation
>> could be made perhaps less bad by having runHooks null out each entry in the
>> hooks table before it runs that hook, or using currentRunningHook to
>> determine which hooks to run in runHooks. Or something else, like immediate
>> exit in this case. (That would be spec change.)
>
>> Is "deadlock" accurate?
>
> Yes.
>
> In the context of the specification, "shutdown hook" means _application_
> shutdown hook - as far as the specification is concerned, application
> shutdown hooks are the only kind of hooks. Right?
>
> For example, the following will deadlock (when run with the changes in this
> PR):
>
>
> public class TestHook {
> public static void main(String... arg) {
> Thread hook = new Thread("my-hook") {
> @Override
> public void run() {
> System.exit(1);
> }
> };
> Runtime.getRuntime().addShutdownHook(hook);
> System.exit(0);
> }
> }
It is a general deadlock, not a monitor based deadlock: the thread that called
exit and holds the lock has to join() the hook thread. The hook thread blocks
on the lock held by the exiting thread. Neither thread can progress.
-
PR: https://git.openjdk.org/jdk/pull/9351
Re: RFR: 8288984: Simplification in Shutdown.exit [v2]
On Mon, 4 Jul 2022 01:59:37 GMT, Kim Barrett wrote: >> src/java.base/share/classes/java/lang/Runtime.java line 89: >> >>> 87: * of the first invocation will be used; the status codes from >>> other invocations >>> 88: * will be ignored. If this method is invoked from a shutdown hook >>> the system >>> 89: * will deadlock. >> >> Expressing this accurately is tricky - what is "first" here? I suggest the >> following: >> >>> Invocations of this method are serialized such that only one invocation >>> will actually proceed with the shutdown sequence and terminate the VM with >>> the given status code. All other invocations will block indefinitely. If >>> this method is invoked from a shutdown hook the system will deadlock. > > +1 - except for the "deadlock" part (see other comment). I think the old > paragraph is at least confusing, and perhaps even just wrong. Let's say > we've run `shutdown` so run all the hooks but not halted. Then someone calls > `exit(0)`. That seems to suggest the call will block indefinitely, which is > neither desirable nor what was actually implemented. David's refinement looks good. The sentence on deadlock is accurate as shutdown hooks run on other threads, not the thread calling System.ext. - PR: https://git.openjdk.org/jdk/pull/9351
Re: RFR: 8288984: Simplification in Shutdown.exit [v2]
On Mon, 4 Jul 2022 01:57:11 GMT, Kim Barrett wrote:
> Is "deadlock" accurate?
Yes.
In the context of the specification, "shutdown hook" means _application_
shutdown hook - as far as the specification is concerned, application shutdown
hooks are the only kind of hooks. Right?
For example, the following will deadlock (when run with the changes in this PR):
public class TestHook {
public static void main(String... arg) {
Thread hook = new Thread("my-hook") {
@Override
public void run() {
System.exit(1);
}
};
Runtime.getRuntime().addShutdownHook(hook);
System.exit(0);
}
}
-
PR: https://git.openjdk.org/jdk/pull/9351
Re: RFR: 8288984: Simplification in Shutdown.exit [v2]
On Sat, 2 Jul 2022 21:21:37 GMT, Ryan Ernst wrote: >> This is a followup to simplify Shutdown.exit after the removal of >> finalizers (https://bugs.openjdk.org/browse/JDK-8198250). Once agreement >> on the approach has been reached in this PR, a CSR will be filed to >> propose the spec change to Runtime.exit. > > Ryan Ernst has updated the pull request incrementally with one additional > commit since the last revision: > > better clarify multiple threads behavior src/java.base/share/classes/java/lang/Runtime.java line 89: > 87: * of the first invocation will be used; the status codes from other > invocations > 88: * will be ignored. If this method is invoked from a shutdown hook > the system > 89: * will deadlock. Is "deadlock" accurate? I thought Java monitors were reentrant, with the result that a shutdown hook calling `exit` would lead to a recursive `runHooks` which would run all the hooks, including *rerunning* hooks before the one that called exit (which could be bad), and then rerunning the hook that called exit (possibly leading to infinite recursion), then running later hooks, then returning to rerun those later hooks. This situation could be made perhaps less bad by having runHooks null out each entry in the hooks table before it runs that hook, or using currentRunningHook to determine which hooks to run in runHooks. Or something else, like immediate exit in this case. (That would be spec change.) - PR: https://git.openjdk.org/jdk/pull/9351
Re: RFR: 8288984: Simplification in Shutdown.exit [v2]
On Sun, 3 Jul 2022 22:19:50 GMT, David Holmes wrote: >> Ryan Ernst has updated the pull request incrementally with one additional >> commit since the last revision: >> >> better clarify multiple threads behavior > > src/java.base/share/classes/java/lang/Runtime.java line 89: > >> 87: * of the first invocation will be used; the status codes from other >> invocations >> 88: * will be ignored. If this method is invoked from a shutdown hook >> the system >> 89: * will deadlock. > > Expressing this accurately is tricky - what is "first" here? I suggest the > following: > >> Invocations of this method are serialized such that only one invocation will >> actually proceed with the shutdown sequence and terminate the VM with the >> given status code. All other invocations will block indefinitely. If this >> method is invoked from a shutdown hook the system will deadlock. +1 - except for the "deadlock" part (see other comment). I think the old paragraph is at least confusing, and perhaps even just wrong. Let's say we've run `shutdown` so run all the hooks but not halted. Then someone calls `exit(0)`. That seems to suggest the call will block indefinitely, which is neither desirable nor what was actually implemented. - PR: https://git.openjdk.org/jdk/pull/9351
Re: RFR: 8288984: Simplification in Shutdown.exit [v2]
On Sat, 2 Jul 2022 21:21:37 GMT, Ryan Ernst wrote: >> This is a followup to simplify Shutdown.exit after the removal of >> finalizers (https://bugs.openjdk.org/browse/JDK-8198250). Once agreement >> on the approach has been reached in this PR, a CSR will be filed to >> propose the spec change to Runtime.exit. > > Ryan Ernst has updated the pull request incrementally with one additional > commit since the last revision: > > better clarify multiple threads behavior Changes requested by dholmes (Reviewer). src/java.base/share/classes/java/lang/Runtime.java line 89: > 87: * of the first invocation will be used; the status codes from other > invocations > 88: * will be ignored. If this method is invoked from a shutdown hook > the system > 89: * will deadlock. Expressing this accurately is tricky - what is "first" here? I suggest the following: > Invocations of this method are serialized such that only one invocation will > actually proceed with the shutdown sequence and terminate the VM with the > given status code. All other invocations will block indefinitely. If this > method is invoked from a shutdown hook the system will deadlock. - PR: https://git.openjdk.org/jdk/pull/9351
Re: RFR: 8288984: Simplification in Shutdown.exit [v2]
On Sat, 2 Jul 2022 21:21:37 GMT, Ryan Ernst wrote: >> This is a followup to simplify Shutdown.exit after the removal of >> finalizers (https://bugs.openjdk.org/browse/JDK-8198250). Once agreement >> on the approach has been reached in this PR, a CSR will be filed to >> propose the spec change to Runtime.exit. > > Ryan Ernst has updated the pull request incrementally with one additional > commit since the last revision: > > better clarify multiple threads behavior Marked as reviewed by kristyle...@github.com (no known OpenJDK username). - PR: https://git.openjdk.org/jdk/pull/9351
Re: RFR: 8288984: Simplification in Shutdown.exit [v2]
On Sat, 2 Jul 2022 19:32:04 GMT, Alan Bateman wrote: >> If a shutdown hook is running, then shutdown has started, right? This new >> wording isn’t really a change, it’s the current behavior (the “otherwise” >> portion of the old wording). But the wording is meant to be more accurate >> for the case of running exit from shutdown hooks. The change is the removal >> of the first case, which is what the code portion of the PR removes. > > The first paragraph of the method description already makes it clear that the > method never returns normally. One option for the third paragraph is to just > remove it, another is to replace it with wording that specifies that the > first thread to call exit is the winner and the exit code provided by other > threads that attempt to exit around the same time (or while exit hooks > execute) will be ignored. I think there is merit with adding a warning that a > shutdown hook invoking exit will deadlock. Thanks for the feedback, they are all good points. I opted to make the behavior with multiple threads more clear, see [8e7d527](https://github.com/openjdk/jdk/pull/9351/commits/8e7d527a182933818bd2d7f0f1b799dac52663be). - PR: https://git.openjdk.org/jdk/pull/9351
Re: RFR: 8288984: Simplification in Shutdown.exit [v2]
> This is a followup to simplify Shutdown.exit after the removal of > finalizers (https://bugs.openjdk.org/browse/JDK-8198250). Once agreement > on the approach has been reached in this PR, a CSR will be filed to > propose the spec change to Runtime.exit. Ryan Ernst has updated the pull request incrementally with one additional commit since the last revision: better clarify multiple threads behavior - Changes: - all: https://git.openjdk.org/jdk/pull/9351/files - new: https://git.openjdk.org/jdk/pull/9351/files/9d972ba6..8e7d527a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=9351=01 - incr: https://webrevs.openjdk.org/?repo=jdk=9351=00-01 Stats: 4 lines in 1 file changed: 1 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/9351.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9351/head:pull/9351 PR: https://git.openjdk.org/jdk/pull/9351
