Review request for 6829503

[Mandy Chung]

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

Thanks
Mandy
> -Alan.
>
>
>
>