Re: RFR: 8288984: Simplification in Shutdown.exit [v3]

2022-07-06 Thread David Holmes
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 [v3]

2022-07-06 Thread David Holmes
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]

2022-07-06 Thread Ryan Ernst
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]

2022-07-06 Thread Ryan Ernst
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]

2022-07-05 Thread David Holmes
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]

2022-07-05 Thread David Holmes
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 [v3]

2022-07-04 Thread Ryan Ernst
> 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