Hi Mandy,
On 22/02/2018 12:28 PM, mandy chung wrote:
On 2/21/18 6:08 PM, David Holmes wrote:
Hi Mandy,
tl;dr: I think this is now good to go.
On 22/02/2018 5:58 AM, mandy chung wrote:
Here is the updated webrev:
http://cr.openjdk.java.net/~mchung/jdk11/webrevs/8198249/webrev.04/
I added some comments to clarify the implementation.
src/java.base/share/classes/java/lang/Shutdown.java
96 if (VM.isShutdown()|| slot <= currentRunningHook)
Nit: need space after ()
Fixed.
113 private static void runHooks() {
114 synchronized (lock) {
115 /* Guard against the possibility of a daemon thread
invoking exit
116 * after DestroyJavaVM initiates the shutdown sequence
117 */
118 if (VM.isShutdown()) return;
119 }
I think this is actually impossible to hit, but that's a separate
cleanup.
When T1 is calling runHooks, T2 calls exit and blocks on Shutdown.class,
once T1 releases the lock if VM does not halt yet, T2 will enter this
method.
Ah! T1 is executing destroyJavaVM - and of course releases the lock
eventually. Got it.
139 // set shutdown state
140 VM.shutdown();
Just an observation that we consider "shutdown" to be after all system
shutdown hooks are run. So this (contrary to what I wrote in the CSR)
narrows the window in which a concurrent exit(-1) would trigger an
immediate halt rather than a hang. Which in turn strengthens the
argument for just dropping that behaviour.
If an application hook calls exit(0), it will hang (not the thread
holding the Shutdown class lock). Do you want to file an issue to track
that?
If an application hook calls exit(0) it will block on the Shutdown.class
lock which then hangs the whole shutdown process as the thread holding
that lock is doing a join() on the hook thread - logical deadlock.
But that wasn't my point at all. I just think this whole "calling exit
with non-zero causes immediate halt" logic is meaningless now there are
no finalizers running (and potentially calling exit).
Cheers,
David
-------
---
src/java.base/share/classes/jdk/internal/misc/VM.java
103 public static void shutdown() {
104 initLevel(SYSTEM_SHUTDOWN);
105 }
Doing this is exactly what I meant by my previous comments.
Good.
Thanks for the thorough review.
Mandy
