Zombie FileHandler locks can exhaust all available log file locks.

[Jason Mehrens]

Daniel,


I agree with you.  I think you have it all covered.


Thanks for working on this!


Jason


----------------------------------------
> Date: Tue, 24 Jun 2014 19:37:24 +0200
> From: daniel.fuchs at oracle.com
> To: jason_mehrens at hotmail.com; alan.bateman at oracle.com; core-libs-dev at openjdk.java.net
> Subject: Re: Zombie FileHandler locks can exhaust all available log file locks.
>
> Hi Jason,
>
> Thanks for the feedback!
>
> On 6/24/14 7:08 PM, Jason Mehrens wrote:
>> Daniel,
>>
>>
>> With regard to JDK-4420020, in the original webrev Jim was incorrectly using java.io.File.canWrite() but that webrev was replaced by the current version. The NIO.2 code performs the effective access checks correctly.
>
> Thanks.
>
>> With regard to JDK-6244047 my concern was that checking the permissions and preconditions up front is a small 'TOCTOU' race and redundant because the next step after that is to open the FileChannel. I would assume both CREATE or CREATE_NEW plus WRITE would perform the effective access checks on open.
>
> OK.
>
>> The use of delete on exit is a deal breaker because you can't undo a registration on close of the FileHandler which can trigger an OOME. See https://bugs.openjdk.java.net/browse/JDK-4872014. I've used custom proxy handlers that generate a file name based off of the LogRecord which results in generating lots of file names and opening and close the FileHandler on demand.
>
> ah. hmmmm. I didn't think there would be that many FileHandlers...
> Well - I guess that if we find a way to reuse the zombie
> lock files then we don't really need the delete on exit.
> Someone creating a FileHandler programmatically should be responsible
> for closing it - so if an application creates FileHandlers without
> closing them then it's a bug in the application.
>
>> If the behavior is reverted to use CREATE instead of CREATE_NEW then we have to deal with JDK-8031438 because any user code can lock a file ahead of opening the FileHandler.
>
> Yes - I discovered that while writing a test for my suggested fix ;-)
> Catching OverlappingFileLockException in FileHandler.openFiles
> and setting available to false when it's thrown fixes the issue.
>
> So let's just remove the deleteOnExit & add the catch for
> OverlappingFileLockException to my patch and we should be
> good.
>
> On the other hand we could just use replace CREATE_NEW by
> CREATE but I have some misgivings about the part that
> sets available to true when tryLock throws an IOException.
> I'm not sure we should be doing that if we haven't actually
> created the lock file. So I think I'd prefer keeping the
> two steps behavior - first try CREATE_NEW - then attempt
> to use CREATE if CREATE_NEW failed...
> I'm not sure CREATE will check the access rights for writing
> in the directory if the file already exists - so checking
> the directory for write access in that case is probably
> the right thing to do.
>
> Would you agree?
>
> -- daniel
>
>>
>> Jason
>>
>>
>>
>> ----------------------------------------
>>> Date: Mon, 23 Jun 2014 17:13:04 +0200
>>> From: daniel.fuchs at oracle.com
>>> To: Alan.Bateman at oracle.com; jason_mehrens at hotmail.com; core-libs-dev at openjdk.java.net
>>> Subject: Re: Zombie FileHandler locks can exhaust all available log file locks.
>>>
>>> On 6/23/14 4:53 PM, Alan Bateman wrote:
>>>> On 23/06/2014 10:48, Daniel Fuchs wrote:
>>>>> :
>>>>>
>>>>> All in all - I feel our best options would be to revert to using
>>>>> CREATE, or use CREATE_NEW + DELETE_ON_CLOSE, or not do anything
>>>>> and live with the issue.
>>>>> Hopefully some nio experts will chime in ;-)
>>>>>
>>>> The logging use of FileLock is a bit unusual but looking at it now then
>>>> I don't see the reason why it needs to use CREATE_NEW.
>>>
>>> OK - thanks - in the discussion that ended up in the patch where
>>> CREATE_NEW was introduced Jason (I think it was Jason) pointed at
>>> https://bugs.openjdk.java.net/browse/JDK-4420020
>>>
>>> I'm guessing here that maybe it would be wrong to use an already
>>> existing lock file if you don't have the rights to write to
>>> its directory - and that using CREATE_NEW ensures that you have
>>> all necessary access rights all along the path.
>>>
>>>> I don't think
>>>> DELETE_ON_CLOSE is suitable here as that work is for transient work
>>>> files which isn't the case here. Instead it could use deleteOnExit to
>>>> have some chance of removing it on a graceful exit.
>>>
>>> Right - I suggested a patch in another mail where I just use that.
>>>
>>>>
>>>> BTW: Do you know why locks is Map? Would a Set do?
>>>
>>> It certainly looks as if we could use a simple HashSet here.
>>>
>>> Thanks Alan!
>>>
>>> -- daniel
>>>
>>>>
>>>> -Alan.
>>>>
>>>
>