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.
