Review request for 6829503

[Alan Bateman]

Mandy Chung wrote:
> 6829503: addShutdownHook fails if called after shutdown has commenced.
>
> Webrev at:
>   http://cr.openjdk.java.net/~mchung/6829503/webrev.00/
>
> I change the Shutdown#add method to take the 
> registerShutdownInProgress parameter.  If set to true, the specified 
> shutdown hook is allowed to be registered while shutdown is in 
> progress.  The method will throw IllegalStateException if the shutdown 
> process already passes this slot.   DeleteOnExitHook is the last 
> shutdown hook to be invoked and it will not be invoked until all 
> application shutdown hooks finish (see 
> ApplicationShutdownHooks.runHooks()).  So any file added to the delete 
> on exit list by the application shutdown hooks will be handled by the 
> DeleteOnExitHook.
>
> The LoggingDeadlock2.java test passes with this fix.  I also add a new 
> jtreg test to exercise the Console and DeleteOnExitHook being 
> initialized during application shutdown.
>
> Alan,
>   I considered your suggestion to make Shutdown#add method to return a 
> boolean instead of checking the state.  I am concerned that if the 
> caller didn't check the return value and handle properly, it would be 
> harder to catch the problem.  So I keep it to check the state and 
> throw IllegalStateException.
>
> Thanks
> Mandy
Good work! It mostly looks good to me and I've only a few comments:

1. I see that registerShutdownHook now throws IAE if the slot is already 
used but if that happens it would be our bug. It can't happen with the 
current code of course but I wonder if an Error might be better (the 
original changes threw an InternalError).

2. File#deleteOnExit doesn't allow IllegalStateException to be thrown so 
maybe we should change DeleteOnExit#add to be a no-op if its shutdown 
hook is running. If an application is attempting to register files to be 
deleted during shutdown it will always be a timing issue if the file is 
deleted or not.

3. In ApplicationShutdownHooks I wonder if it would be cleaner to 
eliminate the static initializer and move the registration attempt into 
the add method. That would avoid needing to catch IllegalStateException 
ie: add becomes:   if (hooks == null) { Shutdown.add(...); hooks = new 
... }. It amounts to the same as what you have now but avoid exception 
catching.

4. The synchronization and checking between Shutdown#add and runHooks 
looks right. A minor comment is that the name "curShutdownHookSlot" is a 
bit inconsistent with the other fields. s/curr/current/ or perhaps a 
different name?

5. I usually prefer to have Runnables be the last parameter, in 
particular for cases like this where anonymous inner classes are used. 
An alternative to introducing the boolean parameter is rename one of the 
register methods, say, registerShutdownHookBeforeShutdown or something 
better.

-Alan.