Thanks Alan.
Mandy
Alan Bateman wrote:
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.
