Review request for 6829503

[Alan Bateman]

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.