Here is the updated webrev:
http://cr.openjdk.java.net/~mchung/jdk11/webrevs/8198249/webrev.04/
I added some comments to clarify the implementation.
Mandy
On 2/21/18 11:34 AM, mandy chung wrote:
Hi David,
I think I'm clear on the implementation now (my mistake that I
neglected ApplicationShutdownHooks). Shutdown class keeps the "system
shutdown hooks". ApplicationShutdownHooks is one system hook that
starts all application shutdown hooks and waits until they finish.
java.io.Console and DeleteOnExitHook are two other system shutdown
hooks to restore console echo state or support deleteOnExit.
The system shutdown hooks are all run in the thread initiating the
shutdown that holds the Shutdown.class.
On 2/20/18 10:27 PM, David Holmes wrote:
Hi Mandy,
On 21/02/2018 5:57 AM, mandy chung wrote:
Hi David,
I reworked the change in Shutdown class and uses
jdk.internal.misc.VM to maintain the shutdown state, either in
progress or shutdown (i.e. all shutdown hooks have been started).
What do you think this revised version:
http://cr.openjdk.java.net/~mchung/jdk11/webrevs/8198249/webrev.03/
It confuses me a bit. On the one hand I wasn't sure why we needed to
bring the VM class into this, then on the other hand it perhaps made
sense to have VM track shutdown states as well as initialization states.
The latter is my motivation and I think it's a good cleanup to have VM
state to indicate it's shutdown.
But I'm not sure about the two-phase "shutdown" state - I think I'd
probably prefer a "shutdown initiated" state and a "shutdown
complete" state. Then no need to duplicate the constant values for
IN_PROGRESS and SHUTDOWN (which now need to be kept in sync with
changes to the VM class). It would also simplify the shutdown() logic.
Shutdown::add method can be used to register a system hook while
shutdown is in progress. Actually currentRunningHook can be used to
guard if Shutdown::add should reject to add a new hook. I updated the
webrev and no longer needs the two phases.
Also shutdown(level) should be using initLevel(value) so that it fits
in with the existing awaitInitLevel() logic and locking strategy!
Someone may want to wait for shutdown in the future. That also deals
with the locking issue in the Shutdown class because you don't need
to use "synchronized(lock)" because runHooks is only called within
"synchronized(Shutdown.class)". [To be fair the existing locking
strategy seems confused to me as well - or at least it confuses me.]
I agree but I tried not to touch the existing locking strategy as it
should be a separate changeset. I prefer to add VM::shutdown method
I also now realize that the changes I suggested for the Runtime.exit
docs was incorrect. The existing docs state that we only halt if
hooks have already been run - which corresponds to the old and new
code. I, for some reason that escapes me, claimed we only cared if
the hooks had been started, not completed - but that is inconsistent
with old spec and the implementation.
I confused myself too. A small change will fix it:
87 * <p> If this method is invoked after all shutdown hooks have already
88 * been run and the status is nonzero then this method halts the
89 * virtual machine with the given status code. Otherwise, this method
Mandy
So apologies but what started out as a seemingly simple code removal,
has become a lot more complicated, and confusing to me.
Thanks,
David
-----
On 2/15/18 9:14 PM, David Holmes wrote:
All other updates seem okay. I did have one further thought - in
Runtime does this change:
public void runFinalization() {
! SharedSecrets.getJavaLangRefAccess().runFinalization();
}
affect the classloading/initialization order at all?
Runtime::runFinalization is not called by the system code. So it
won't be invoked during startup and hence won't change the
classloading order during startup.
Mandy
