Re: Review request for 6829503
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.
Re: Review request for 6829503
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.
Re: Review request for 6829503
David Holmes - Sun Microsystems wrote: Hi Alan, Alan Bateman said the following on 04/20/09 00:00: As it happens, there was a bug in that code (6526376) that caused NPE to be thrown so jdk7 b10 is the first build where IllegalStateException is possible. Ah I see. Seems to me that 6526376 took the easy fix and ignored the underlying issue of whether this should be an exceptional circumstance or not. :( or probably earlier in 4809375 when there was code introduced to explicitly handle this case. : I think there's probably a way to keep the list in java code (for resource management) and also add the fd to list used by the VM. Adding a new VM shutdown hook to be executed by the VMThread at the termination safepoint would probably be the best place to deal with this. That would work but does mean another dependency in the VM code. I'll create a bug with a link to this discussion so that this issue doesn't get forgotten. -Alan.
Re: Review request for 6829503
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.
Re: Review request for 6829503
Alan, 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. But a runtime exception is better than silent failure when the file will not be deleted. And the exception here is not new. Is this a flaw in the File#deleteOnExit API - because it gives the illusion that it can always succeed when in fact the code requesting this can be running concurrently with the code responsible for making it happen? Maybe that is a deficiency of the whole mechanism - really we should be using a native deletion mechanism that is only done once the VM arrives at the termination safepoint - as that is the only time we know for sure that no more Java code can be executed. But that would be a RFE on the deleteOnExit mechanism. Cheers, David
Re: Review request for 6829503
Martin Buchholz a écrit : ... It seems to me there is an inherent race condition with such a check-then-act sequence, unless there is a mechanism for the application to hold some kind of shutdown hook lock. Martin I don't think so. The idea is just to prevent users to use some methods in a shutdown hook when the VM terminates. It's rather a chek-then-don't act. Rémi
Re: Review request for 6829503
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. 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
Re: Review request for 6829503
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
Re: Review request for 6829503
Mandy Chung wrote:
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
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).
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.
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.
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?
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.
-Alan.
Re: Review request for 6829503
On Fri, Apr 17, 2009 at 22:53, Mandy Chung mandy.ch...@sun.com wrote: Hi Martin, Thanks for the quick review. Users should only add their shutdown hooks via the System.addShutdownHook() method. java.lang.Shutdown is an implementation class for registering internal hooks besides application shutdown hooks - including the shutdown hook for Console and DeleteOnExitHook. Applications should not use the Shutdown#add method. I think you misunderstood me. I was musing about how, in the future, we could allow applications to also manipulate hooks during shutdown, using some kind of similar mechanism as done with internal hooks. Almost off-topic on my part. Martin
Re: Review request for 6829503
On Sat, Apr 18, 2009 at 06:20, Rémi Forax fo...@univ-mlv.fr wrote: The other solution is to provide a way in the API to test if shutdown hooks have been started or not. In that case, all application codes that start a shutdown hook like Console.readPassword() can check if shudown hooks run or not and throw a runtime exception if shutdown hook run. It seems to me there is an inherent race condition with such a check-then-act sequence, unless there is a mechanism for the application to hold some kind of shutdown hook lock. Martin
Re: Review request for 6829503
Thanks for your quick response on this. A quick review says: looks good to me. Someone should give it a more thorough review. - The solution of allowing shutdown hooks of a particular type to be added during shutdown before that slot is reached is clever. I'm sure there are use cases for users to be able to do the same thing with their own shutdown hooks, but it would be tricky to provide such a facility in a clean way. How do you manage the order of independently developed shutdown hooks? Probably still a research problem. - the last shutdown hook to invoke. = the last shutdown hook to be invoked. Martin On Fri, Apr 17, 2009 at 15:05, Mandy Chung mandy.ch...@sun.com wrote: 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
Re: Review request for 6829503
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
Re: Review request for 6829503
Hi Martin, Thanks for the quick review. Users should only add their shutdown hooks via the System.addShutdownHook() method. java.lang.Shutdown is an implementation class for registering internal hooks besides application shutdown hooks - including the shutdown hook for Console and DeleteOnExitHook. Applications should not use the Shutdown#add method. How do you manage the order of independently developed shutdown hooks? I assume you're concerned with application shutdown hooks. The order will be managed by ApplicationShutdownHooks (in the order when a hook is added). Are you concerned with other use of Shutdown#add? Mandy Martin Buchholz wrote: Thanks for your quick response on this. A quick review says: looks good to me. Someone should give it a more thorough review. - The solution of allowing shutdown hooks of a particular type to be added during shutdown before that slot is reached is clever. I'm sure there are use cases for users to be able to do the same thing with their own shutdown hooks, but it would be tricky to provide such a facility in a clean way. How do you manage the order of independently developed shutdown hooks? Probably still a research problem. - the last shutdown hook to invoke. = the last shutdown hook to be invoked. Martin On Fri, Apr 17, 2009 at 15:05, Mandy Chung mandy.ch...@sun.com wrote: 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
