Review request for 6829503

[Xueming Shen]

Mandy Chung wrote:
> David,
>
> Thanks for the review.
>
> Whether the application shutdown hooks should always be invoked first 
> is a good question. 
> The Console shutdown hook is added to restore the console after 
> prompting for a password to fix:
>  6363043 Console will not return to original state when the process is 
> killed with Ctrl+C (sol)
>
> I consulted with Sherman some time back about the shutdown hook 
> ordering.  The Console restores the state before the application 
> shutdown hook is to enable the application shutdown hooks that use 
> Console to work properly.
>
> While working on this fix, I also observed the issue you point out - 
> we won't restore the state of the console if the first use of the 
> console is by an application shutdown or by some other thread when 
> shutdown has commenced.   It is only an issue when it gets terminated 
> in the middle of reading the password.   Normal return of the 
> Console.readPassword method will restore the state.
>
> I consulted with Alan.   He points out that it would go against all 
> recommendations to prompt for a password in a shutdown hook.  He 
> suggests to leave the existing behavior as it is.
>
> Alan, Sherman,
>    Do you have any comments on the order of Console shutdown hook and 
> application shutdown hooks?  It seems sensible that the application 
> shutdown hooks should run first and then other "internal hooks" as 
> David said.
Two use scenarios

(1) jvm is being terminated "abnormally" during a password reading 
operation (during "normal" application
operation), in which the echo is in "off" mode when these shutdown hooks 
start to be invoked. Without
the "console restore first", application hooks probably can do nothing 
with the stdin/console

(2)Application hooks use Console.readPassword() and then be terminated 
"abnormally" during the reading,
in which the echo mode is kept in "off" mode and will not be restored 
after app exits.

The reason the console hook was/is added to run first is we weighed the 
first scenario as a more likely
scenario in real world application. Note, the Console echo mode is not 
required to be "restored" if the
reading operation finishes normally.

Maybe we can consider to run the console hook again (if the hook is set) 
after the application hooks?

Sherman


>
> Thanks
> Mandy
>
> David Holmes - Sun Microsystems wrote:
>> Hi Mandy,
>>
>> Looks good but I have one query.
>>
>> At the top-level there are 3 shutdown hooks:
>>
>> - console hook
>> - application hooks
>> - deleteOnExit hook
>>
>> and they run in this order. The deleteOnExit hook can be added when 
>> shutdown is in progress, so this allows first-use of deleteOnExit 
>> during an application shutdown hook. Okay - that's fine and solves 
>> current problem very neatly.
>>
>> But the Console hook that restores the echo state runs first, so if 
>> the first use of the console is by an application shutdown hook, or 
>> by some other thread when shutdown has commenced, then we won't 
>> restore the state of the console. I guess that's how it currently 
>> works anyway, but it seems strange. I'm not familiar with the Console 
>> but I would have expected (naively perhaps) the application shutdown 
>> hooks to run first and then any "internal hooks". That way we would 
>> always restore the state (though I guess there is potentially a race 
>> with active threads still using the console).
>>
>> David
>>
>> Mandy Chung said the following on 04/18/09 08:05:
>>> 6829503: addShutdownHook fails if called after shutdown has commenced.
>>>
>>> Webrev at:
>>>   http://cr.openjdk.java.net/~mchung/6829503/webrev.00/
>>>
>>> I change the Shutdown#add method to take the 
>>> registerShutdownInProgress parameter.  If set to true, the specified 
>>> shutdown hook is allowed to be registered while shutdown is in 
>>> progress.  The method will throw IllegalStateException if the 
>>> shutdown process already passes this slot.   DeleteOnExitHook is the 
>>> last shutdown hook to be invoked and it will not be invoked until 
>>> all application shutdown hooks finish (see 
>>> ApplicationShutdownHooks.runHooks()).  So any file added to the 
>>> delete on exit list by the application shutdown hooks will be 
>>> handled by the DeleteOnExitHook.
>>>
>>> The LoggingDeadlock2.java test passes with this fix.  I also add a 
>>> new jtreg test to exercise the Console and DeleteOnExitHook being 
>>> initialized during application shutdown.
>>>
>>> Alan,
>>>   I considered your suggestion to make Shutdown#add method to return 
>>> a boolean instead of checking the state.  I am concerned that if the 
>>> caller didn't check the return value and handle properly, it would 
>>> be harder to catch the problem.  So I keep it to check the state and 
>>> throw IllegalStateException.
>>>
>>> Thanks
>>> Mandy
>