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 [v3]
On Tue, 5 Jul 2022 04:47:40 GMT, Ryan Ernst wrote: >> First, the signal handlers actually invoke Shutdown.exit, not >> Shutdown.shutdown - see Terminator.setup(). (If they did the latter, like >> DestroyJavaVM, then an exit(status) call from another thread could still do >> the final VM halt(status) >> >> But now you are opening a can of worms. There are multiple ways for the VM >> to initiate termination - are you going to try and describe here how all of >> them potentially interact? >> >> What you are really stating here is that other parts of the JDK can invoke >> System.exit, but that is for them to specify where such things are >> specified, it isn't for exit() to try and list them all. All we have to do >> here is describe how multiple calls to exit() behave. > > Aha! I think I misread the comment on Shutdown.shutdown and had it stuck in > my head that signals would exit through that method. Thanks for the > explanation, it makes a lot more sense now. > >> All we have to do here is describe how multiple calls to exit() behave. > > The thing that is still missing to me is that an application developer may > have a single call to System.exit, yet the status code they pass to it may > not be the one the jvm exits with. Yes and the problem is? If the user kills the VM by other means; or the termination of the last non-daemon thread races with a call to System.exit then the developer doesn't really have to right to expect anything in relation to that one call. - 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 [v3]
On Tue, 5 Jul 2022 03:24:28 GMT, Ryan Ernst wrote: >> src/java.base/share/classes/java/lang/Runtime.java line 88: >> >>> 86: * Shutdown is serialized such that only one invocation will run >>> 87: * shutdown hooks and terminate the VM with the given status code. >>> That >>> 88: * invocation may be initiated via platform specific signal >>> handlers. All >> >> Why are we mentioning signal handlers here? How is that relevant? > > Signal handlers for eg SIGTERM invoke Shutdown.shutdown. That method holds > the same lock as Shutdown.exit and runs shutdown hooks. So if a signal > handler triggers shutdown, and before the system halts Runtime.exit is > invoked, the status passed to Runtime.exit will be ignored. First, the signal handlers actually invoke Shutdown.exit, not Shutdown.shutdown - see Terminator.setup(). (If they did the latter, like DestroyJavaVM, then an exit(status) call from another thread could still do the final VM halt(status) But now you are opening a can of worms. There are multiple ways for the VM to initiate termination - are you going to try and describe here how all of them potentially interact? What you are really stating here is that other parts of the JDK can invoke System.exit, but that is for them to specify where such things are specified, it isn't for exit() to try and list them all. All we have to do here is describe how multiple calls to exit() behave. - PR: https://git.openjdk.org/jdk/pull/9351
Re: RFR: 8288984: Simplification in Shutdown.exit [v3]
On Tue, 5 Jul 2022 04:06:03 GMT, David Holmes wrote: >> Signal handlers for eg SIGTERM invoke Shutdown.shutdown. That method holds >> the same lock as Shutdown.exit and runs shutdown hooks. So if a signal >> handler triggers shutdown, and before the system halts Runtime.exit is >> invoked, the status passed to Runtime.exit will be ignored. > > First, the signal handlers actually invoke Shutdown.exit, not > Shutdown.shutdown - see Terminator.setup(). (If they did the latter, like > DestroyJavaVM, then an exit(status) call from another thread could still do > the final VM halt(status) > > But now you are opening a can of worms. There are multiple ways for the VM to > initiate termination - are you going to try and describe here how all of them > potentially interact? > > What you are really stating here is that other parts of the JDK can invoke > System.exit, but that is for them to specify where such things are specified, > it isn't for exit() to try and list them all. All we have to do here is > describe how multiple calls to exit() behave. Aha! I think I misread the comment on Shutdown.shutdown and had it stuck in my head that signals would exit through that method. Thanks for the explanation, it makes a lot more sense now. > All we have to do here is describe how multiple calls to exit() behave. The thing that is still missing to me is that an application developer may have a single call to System.exit, yet the status code they pass to it may not be the one the jvm exits with. - PR: https://git.openjdk.org/jdk/pull/9351
Re: RFR: 8288984: Simplification in Shutdown.exit [v3]
On Tue, 5 Jul 2022 00:16:50 GMT, David Holmes wrote: >> Ryan Ernst has updated the pull request incrementally with one additional >> commit since the last revision: >> >> iterate on wording > > src/java.base/share/classes/java/lang/Runtime.java line 88: > >> 86: * Shutdown is serialized such that only one invocation will run >> 87: * shutdown hooks and terminate the VM with the given status code. >> That >> 88: * invocation may be initiated via platform specific signal >> handlers. All > > Why are we mentioning signal handlers here? How is that relevant? Signal handlers for eg SIGTERM invoke Shutdown.shutdown. That method holds the same lock as Shutdown.exit and runs shutdown hooks. So if a signal handler triggers shutdown, and before the system halts Runtime.exit is invoked, the status passed to Runtime.exit will be ignored. - PR: https://git.openjdk.org/jdk/pull/9351
Re: RFR: 8288984: Simplification in Shutdown.exit [v3]
On Mon, 4 Jul 2022 16:17:27 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: > > iterate on wording src/java.base/share/classes/java/lang/Runtime.java line 90: > 88: * invocation may be initiated via platform specific signal handlers. > All > 89: * other invocations will block indefinitely, and their supplied exit > 90: * statuses will be ignored. If this method is invoked from a > shutdown hook If they block indefinitely then it is implicit that nothing happens with their exit status. I think you are trying to say too much. - PR: https://git.openjdk.org/jdk/pull/9351
Re: RFR: 8288984: Simplification in Shutdown.exit [v3]
On Mon, 4 Jul 2022 16:17:27 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: > > iterate on wording src/java.base/share/classes/java/lang/Runtime.java line 88: > 86: * Shutdown is serialized such that only one invocation will run > 87: * shutdown hooks and terminate the VM with the given status code. > That > 88: * invocation may be initiated via platform specific signal handlers. > All Why are we mentioning signal handlers here? How is that relevant? - 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 [v3]
> 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: iterate on wording - Changes: - all: https://git.openjdk.org/jdk/pull/9351/files - new: https://git.openjdk.org/jdk/pull/9351/files/8e7d527a..2253259c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=9351=02 - incr: https://webrevs.openjdk.org/?repo=jdk=9351=01-02 Stats: 6 lines in 1 file changed: 2 ins; 0 del; 4 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
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
On Sat, 2 Jul 2022 13:23:27 GMT, David Holmes wrote: > Sorry, I did not think this issue was intended to change the specification in > any way, but I see now that it actually does - whether that was the intent or > not. While, maybe, not strictly envisioned by the JIRA issue, the specification clarification is IMO a good addition. We should update the synopsis to make it clear that the simplification is to j.l.Runtime::exit. - 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
Re: RFR: 8288984: Simplification in Shutdown.exit
On Fri, 1 Jul 2022 20:01:48 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. 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
On Sat, 2 Jul 2022 14:07:57 GMT, Ryan Ernst wrote: >> src/java.base/share/classes/java/lang/Runtime.java line 88: >> >>> 86: * Invocations of this method block indefinitely. It is >>> therefore >>> 87: * inadvisable to invoke this method from a shutdown hook as it will >>> 88: * cause deadlock. >> >> This is inaccurate. The method only blocks indefinitely when shutdown has >> already commenced. > > 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. - PR: https://git.openjdk.org/jdk/pull/9351
Re: RFR: 8288984: Simplification in Shutdown.exit
On Sat, 2 Jul 2022 13:21:06 GMT, David Holmes 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. > > src/java.base/share/classes/java/lang/Runtime.java line 88: > >> 86: * Invocations of this method block indefinitely. It is therefore >> 87: * inadvisable to invoke this method from a shutdown hook as it will >> 88: * cause deadlock. > > This is inaccurate. The method only blocks indefinitely when shutdown has > already commenced. 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. - PR: https://git.openjdk.org/jdk/pull/9351
Re: RFR: 8288984: Simplification in Shutdown.exit
On Fri, 1 Jul 2022 20:01:48 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. Sorry, I did not think this issue was intended to change the specification in any way, but I see now that it actually does - whether that was the intent or not. src/java.base/share/classes/java/lang/Runtime.java line 88: > 86: * Invocations of this method block indefinitely. It is therefore > 87: * inadvisable to invoke this method from a shutdown hook as it will > 88: * cause deadlock. This is inaccurate. The method only blocks indefinitely when shutdown has already commenced. - PR: https://git.openjdk.org/jdk/pull/9351
Re: RFR: 8288984: Simplification in Shutdown.exit
On Fri, 1 Jul 2022 20:01:48 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. There is not supposed to be any spec change in relation to this issue. The behaviour should be as specified; it was the coding that was intended to be simplified. - PR: https://git.openjdk.org/jdk/pull/9351
