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.fu...@oracle.com > To: jason_mehr...@hotmail.com; alan.bate...@oracle.com; > core-libs-dev@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.fu...@oracle.com >>> To: alan.bate...@oracle.com; jason_mehr...@hotmail.com; >>> core-libs-dev@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. >>>> >>> >
