Review request for 6829503

[Mandy Chung]

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.