Zombie FileHandler locks can exhaust all available log file locks. (original) (raw)

Daniel Fuchs daniel.fuchs at oracle.com
Tue Jun 24 17:37:24 UTC 2014

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 CREATENEW 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 CREATENEW 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; jasonmehrens 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 CREATENEW + DELETEONCLOSE, 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 CREATENEW. OK - thanks - in the discussion that ended up in the patch where CREATENEW 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 CREATENEW ensures that you have all necessary access rights all along the path. I don't think DELETEONCLOSE 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.

More information about the core-libs-dev mailing list