Now that I understand what this is for I see the predicament. Basically
there's no single correct action because it all depends on what the
application was doing, and will do, during shutdown.
The current behaviour handles the most likely case of hitting ctrl-c
while the password is being prompted for.
Running the console hook before and after the application hook, would be
slightly "safer" but I don't think it's really worth the effort.
Seems to me the fundamental problem here is the semantics for concurrent
use of the Console ... but that's a different issue.
So I have no issue with the fix as-is.
Thanks,
David
Xueming Shen said the following on 04/18/09 16:51:
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
