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.
