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.
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?
---
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