Re: Review Request (1-line fix): 7032589 FileHandler leaking file descriptor of the file lock

2011-04-16 Thread Alan Bateman
Mandy Chung wrote: : I created a CR 7037134: Potential writing to the same log file by multiple processes I don't see anything in the logging API that requires the log file to be locked but my guess is that this code is there for cases where the log file is on a NFS server and the lock daemon

Review Request (1-line fix): 7032589 FileHandler leaking file descriptor of the file lock

2011-04-15 Thread Mandy Chung
Fix for 7032589: FileHandler leaking file descriptor of the file lock Webrev at: http://cr.openjdk.java.net/~mchung/jdk7/7032589/webrev.00/ When a log file is currently being used, tryLock() will fail and it has to close the file channel of the lock file Thanks Mandy

Re: Review Request (1-line fix): 7032589 FileHandler leaking file descriptor of the file lock

2011-04-15 Thread Daniel D. Daugherty
Thumbs up. Dan On 4/15/2011 11:01 AM, Mandy Chung wrote: Fix for 7032589: FileHandler leaking file descriptor of the file lock Webrev at: http://cr.openjdk.java.net/~mchung/jdk7/7032589/webrev.00/ When a log file is currently being used, tryLock() will fail and it has to close the file

Re: Review Request (1-line fix): 7032589 FileHandler leaking file descriptor of the file lock

2011-04-15 Thread Rémi Forax
On 04/15/2011 07:55 PM, Daniel D. Daugherty wrote: Thumbs up. Dan Hi Mandy, if fc.close() throws an IOException , it goes wrong. I think you need to put fc.close() in its own a try/catch(IOException). Rémi On 4/15/2011 11:01 AM, Mandy Chung wrote: Fix for 7032589: FileHandler leaking

Re: Review Request (1-line fix): 7032589 FileHandler leaking file descriptor of the file lock

2011-04-15 Thread Mandy Chung
Hi Remi, On 04/15/11 11:05, Rémi Forax wrote: Hi Mandy, if fc.close() throws an IOException , it goes wrong. I think you need to put fc.close() in its own a try/catch(IOException). That's a good point. I think it should throw IOException if anything goes wrong with fc.close(). Revised the

Re: Review Request (1-line fix): 7032589 FileHandler leaking file descriptor of the file lock

2011-04-15 Thread Daniel D. Daugherty
On 4/15/2011 1:12 PM, Mandy Chung wrote: Hi Remi, On 04/15/11 11:05, Rémi Forax wrote: Hi Mandy, if fc.close() throws an IOException , it goes wrong. I think you need to put fc.close() in its own a try/catch(IOException). That's a good point. I think it should throw IOException if

Re: Review Request (1-line fix): 7032589 FileHandler leaking file descriptor of the file lock

2011-04-15 Thread Mandy Chung
On 04/15/11 13:41, Daniel D. Daugherty wrote: On 4/15/2011 1:12 PM, Mandy Chung wrote: Hi Remi, On 04/15/11 11:05, Rémi Forax wrote: Hi Mandy, if fc.close() throws an IOException , it goes wrong. I think you need to put fc.close() in its own a try/catch(IOException). That's a good point.

Re: Review Request (1-line fix): 7032589 FileHandler leaking file descriptor of the file lock

2011-04-15 Thread Rémi Forax
On 04/15/2011 10:55 PM, Mandy Chung wrote: On 04/15/11 13:41, Daniel D. Daugherty wrote: On 4/15/2011 1:12 PM, Mandy Chung wrote: Hi Remi, On 04/15/11 11:05, Rémi Forax wrote: Hi Mandy, if fc.close() throws an IOException , it goes wrong. I think you need to put fc.close() in its own a

Re: Review Request (1-line fix): 7032589 FileHandler leaking file descriptor of the file lock

2011-04-15 Thread Mandy Chung
On 4/15/11 3:57 PM, Rémi Forax wrote: Hi Mandy, Minor nit, initializing available when declaring it is not necessary because both path (with or without exception) initialize it later. Sure. Will fix that before the push. otherwise, I'm Ok with the patch. Thanks Mandy