Review request for 6829503

2009-04-17 Thread Mandy Chung
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

Re: Review request for 6829503

2009-04-17 Thread Martin Buchholz
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 user

Re: Review request for 6829503

2009-04-17 Thread David Holmes - Sun Microsystems
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 appli

Re: Review request for 6829503

2009-04-17 Thread Mandy Chung
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 DeleteOn

Re: Review request for 6829503

2009-04-17 Thread Mandy Chung
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 ki

Re: Review request for 6829503

2009-04-17 Thread 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

Re: Review request for 6829503

2009-04-18 Thread David Holmes - Sun Microsystems
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 prom

Re: Review request for 6829503

2009-04-18 Thread Rémi Forax
Xueming Shen a écrit : [...] 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", applicati

Re: Review request for 6829503

2009-04-18 Thread Alan Bateman
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 allow

Re: Review request for 6829503

2009-04-18 Thread Martin Buchholz
On Fri, Apr 17, 2009 at 22:53, Mandy Chung 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 ho

Re: Review request for 6829503

2009-04-18 Thread Martin Buchholz
On Sat, Apr 18, 2009 at 06:20, Rémi Forax 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 thr

Re: Review request for 6829503

2009-04-19 Thread David Holmes - Sun Microsystems
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

Re: Review request for 6829503

2009-04-19 Thread Alan Bateman
David Holmes - Sun Microsystems wrote: : 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 req

Re: Review request for 6829503

2009-04-19 Thread Rémi Forax
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 s

Re: Review request for 6829503

2009-04-19 Thread David Holmes - Sun Microsystems
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 underlyi

Re: Review request for 6829503

2009-04-20 Thread Alan Bateman
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 too

Re: Review request for 6829503

2009-04-20 Thread Mandy Chung
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 re

Re: Review request for 6829503

2009-04-21 Thread 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 comment

Re: Review request for 6829503

2009-04-21 Thread 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 lo