Mandy Chung wrote:
Alan,
Thanks for the review and comments. Here is the revised webrev
incorporating your comments. See below for my inlined reply.
http://cr.openjdk.java.net/~mchung/6829503/webrev.01/
Alan Bateman wrote:
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).
I changed it to throw an InternalError. Since it's an implementation
error, Error is appropriate.
Yes, this is better.
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.
Thanks for filing the RFE. No change is needed for this fix.
Sorry, this tangent delayed the push. Hopefully you will be able to get
it approved for one of the remaining builds for M3.
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.
I prefer to leave it as it is.
We could move the registration to the add and remove method. We
would have to add another state to indicate if the shutdown hook is
running vs the shutdown hook is not registered. So we can't just do
if (hooks == null) { Shutdown.add(...); ... }? When the application
shutdown hooks are running, hooks is set to null. When a shutdown
hook attempts to call Runtime.addShutdownHook that will end up getting
an InternalError() since the hook at slot 1 has been registered.
Ah yes, it would be an InternalError rather than the
IllegalStateException. In that case leave it as is.
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?
How about currentRunningHook?
Yes, looks better.
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.
I moved the Runnable parameter to the last parameter. I also remove
the JavaLangAccess.registerShutdownHook(int, Runnable) method which I
don't think it's highly necessary. So Console and DeleteOnExitHook
both will need to specify the boolean parameter.
If there are other changes in the future that add further complexity
that it might be good to have different register names. For now it's all
looks okay to me.
-Alan.
