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

[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 anything 
> goes wrong with fc.close(). Revised the fix:
>     http://cr.openjdk.java.net/~mchung/jdk7/7032589/webrev.01/

Thumbs up on this version.


>
> The existing implementation looks a bit suspicious as it silently 
> ignores the IOException thrown by tryLock() and uses that lock file.

I think that the existing code is remembering the lock file name
even which IOOException is thrown because of the assumption that
IOException is only thrown when locking is not supported on the
target directory. By remembering the lock file name, any further
attempt to lock the file by the same process doesn't result in
a another tryLock() call with another resulting IOException.

It is troubling that this code is giving a false assurance that
the file system object is locked when it isn't. In the case where
an IOException was thrown by tryLock(), the file system object is
only locked in the context of the current process. That seems to
make it easy for two different processes to both think that they
have the file system object locked. Potential train wreck down the
road? Quite possibly.


> I am inclined to leave the existing behavior as it is to minimize 
> behavioral change.  And just resolve this file descriptor leak issue 
> in this fix.

Agreed that this bug should address the leak and that a new
bug should be opened for the possible locking issue.

Dan


> I'll open a bug to follow up this.
>
>  412                 try {
>  413                     FileLock fl = fc.tryLock();
>  414                     if (fl == null) {
>  415                         // We failed to get the lock.  Try next 
> file.
>  416                         continue;
>  417                     }
>  418                     // We got the lock OK.
>  419                 } catch (IOException ix) {
>  420                     // We got an IOException while trying to get 
> the lock.
>  421                     // This normally indicates that locking is 
> not supported
>  422                     // on the target directory.  We have to 
> proceed without
>  423                     // getting a lock.   Drop through.
>  424                 }
>
> Mandy
>
>
>