Re: RFR: 6244047: impossible to specify directories to logging FileHandler unless they exist

2012-11-15 Thread Alan Bateman 

On 14/11/2012 21:54, Jim Gish wrote:


On 11/14/2012 04:38 PM, Alan Bateman wrote:

On 14/11/2012 20:56, Jim Gish wrote:
Check out the latest, please -- 
http://cr.openjdk.java.net/~jgish/Bug6244047-FileHandler-CheckLockLocation/ 
 
-- If it's ok, please push it or let me know who to have do it?
I think it's okay except that you don't need to catch IOException, 
simply catching FileAlreadyExistsException exception should do it. If 
you agree then update the patch and I can push it for you.
But of course.  Sorry I missed the obvious.  Just peeling the onion 
when I could have taken a bite :-)  Fixed, and updated.

Good, it looks fine now.





Exactly -- that's my point.  This is one of those cases.  You're 
trying to create a new file in a directory, but the file you specified 
isn't a directory - it's a plain file.  The error code coming back is 
ENOTDIR and so what you get is the more general FileSystemException 
with "Not a directory" as the error message, when in fact, we could 
throw NotDirectoryException if we'd simply check for errno of ENOTDIR 
in the first place along with the other checks being done in 
UnixException translateToIOException(File,String).
This isn't an obvious as it might seem because having ENOTDIR always map 
to DirectoryNotEmptyException may cause that exception to be thrown in 
other cases too.  Additionally, this specific exception was intended for 
cases where you attempt to do something on a directory but it turns out 
the file is not a directory, this is subtly different to the case here. 
So we need to separate this one, I think the FileSystemException that 
you are seeing now is reasonable.


-Alan

Re: RFR: 6244047: impossible to specify directories to logging FileHandler unless they exist

2012-11-14 Thread Jim Gish 


On 11/14/2012 04:38 PM, Alan Bateman wrote:

On 14/11/2012 20:56, Jim Gish wrote:
Check out the latest, please -- 
http://cr.openjdk.java.net/~jgish/Bug6244047-FileHandler-CheckLockLocation/ 
 
-- If it's ok, please push it or let me know who to have do it?
I think it's okay except that you don't need to catch IOException, 
simply catching FileAlreadyExistsException exception should do it. If 
you agree then update the patch and I can push it for you.
But of course.  Sorry I missed the obvious.  Just peeling the onion when 
I could have taken a bite :-)  Fixed, and updated.



:

BTW I was expecting that NotDirectoryException would be thrown. 
However, sun.nio.fs.UnixException does not translate an error code 20 
(UnixConstants.ENOTDIR) to NotDirectoryException, even though it 
could.  Perhaps we should fix that, unless you see a reason not to.  
I'll check the history, bug reports, etc. and bring it up on nio-dev 
unless you know off the top of your head why we're not checking for 
ENOTDIR error code.
NotDirectoryException is for the case where you attempt do something 
specific to a directory but the file isn't a directory. There is 
special handing in newDirectoryStream for this.


Exactly -- that's my point.  This is one of those cases.  You're trying 
to create a new file in a directory, but the file you specified isn't a 
directory - it's a plain file.  The error code coming back is ENOTDIR 
and so what you get is the more general FileSystemException with "Not a 
directory" as the error message, when in fact, we could throw 
NotDirectoryException if we'd simply check for errno of ENOTDIR in the 
first place along with the other checks being done in UnixException 
translateToIOException(File,String).



-Alan.


--
Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304
Oracle Java Platform Group | Core Libraries Team
35 Network Drive
Burlington, MA 01803
jim.g...@oracle.com

Re: RFR: 6244047: impossible to specify directories to logging FileHandler unless they exist

2012-11-14 Thread Alan Bateman 

On 14/11/2012 20:56, Jim Gish wrote:
Check out the latest, please -- 
http://cr.openjdk.java.net/~jgish/Bug6244047-FileHandler-CheckLockLocation/ 
 
-- If it's ok, please push it or let me know who to have do it?
I think it's okay except that you don't need to catch IOException, 
simply catching FileAlreadyExistsException exception should do it. If 
you agree then update the patch and I can push it for you.



:

BTW I was expecting that NotDirectoryException would be thrown.  
However, sun.nio.fs.UnixException does not translate an error code 20 
(UnixConstants.ENOTDIR) to NotDirectoryException, even though it 
could.  Perhaps we should fix that, unless you see a reason not to.  
I'll check the history, bug reports, etc. and bring it up on nio-dev 
unless you know off the top of your head why we're not checking for 
ENOTDIR error code.
NotDirectoryException is for the case where you attempt do something 
specific to a directory but the file isn't a directory. There is special 
handing in newDirectoryStream for this.


-Alan.

Re: RFR: 6244047: impossible to specify directories to logging FileHandler unless they exist

2012-11-14 Thread Jim Gish 
Check out the latest, please -- 
http://cr.openjdk.java.net/~jgish/Bug6244047-FileHandler-CheckLockLocation/ 
 
-- If it's ok, please push it or let me know who to have do it?


Thanks,
Jim

BTW I was expecting that NotDirectoryException would be thrown. However, 
sun.nio.fs.UnixException does not translate an error code 20 
(UnixConstants.ENOTDIR) to NotDirectoryException, even though it could.  
Perhaps we should fix that, unless you see a reason not to. I'll check 
the history, bug reports, etc. and bring it up on nio-dev unless you 
know off the top of your head why we're not checking for ENOTDIR error code.



On 11/14/2012 06:08 AM, Alan Bateman wrote:

On 13/11/2012 21:30, Jim Gish wrote:
Here's a new webrev with my latest changes for your reviewing 
pleasure :-)


http://cr.openjdk.java.net/~jgish/Bug6244047-FileHandler-CheckLockLocation/ 



Main changes:
- Using the file channel directly as suggested.
- Instead of checking up front for a valid directory I check the 
IOException on the channel open and process it accordingly. (BTW, I 
much prefer my previous proposed fix because I like to ensure 
pre-conditions for an operation are met prior to it rather than 
attempting the op, failing, and then recovering),
- Eliminated the stream, which really isn't needed, and just use the 
file channel


Just for the purposes of my enlightenment, assuming this is what you 
were after (Jason & Alan), what was your issue with checking for a 
valid directory up-front?


Thanks,
   Jim
I get it now (I missed this on the first round), this code is using 
lock files and not really using file-locking as intended.


I think the FileChannel.open usage is fine, I'm just not sure about 
the exception handling. For starters, FileSystemException is a super 
type of AccessDeniedException and NoSuchFileSystem so I don't think 
you need to catch them specifically. Would I be correct to say that 
the only reason that you would have to recover and try the next file 
is if the lock file exist? In that case then maybe it just needs to be:


try {
lockFileChannel = FileChannel.open(...);
} catch (FileAlreadyExistsException ignore) {
continue;
}

-Alan.






--
Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304
Oracle Java Platform Group | Core Libraries Team
35 Network Drive
Burlington, MA 01803
jim.g...@oracle.com

Re: RFR: 6244047: impossible to specify directories to logging FileHandler unless they exist

2012-11-14 Thread Alan Bateman 

On 13/11/2012 21:30, Jim Gish wrote:

Here's a new webrev with my latest changes for your reviewing pleasure :-)

http://cr.openjdk.java.net/~jgish/Bug6244047-FileHandler-CheckLockLocation/ 



Main changes:
- Using the file channel directly as suggested.
- Instead of checking up front for a valid directory I check the 
IOException on the channel open and process it accordingly. (BTW, I 
much prefer my previous proposed fix because I like to ensure 
pre-conditions for an operation are met prior to it rather than 
attempting the op, failing, and then recovering),
- Eliminated the stream, which really isn't needed, and just use the 
file channel


Just for the purposes of my enlightenment, assuming this is what you 
were after (Jason & Alan), what was your issue with checking for a 
valid directory up-front?


Thanks,
   Jim
I get it now (I missed this on the first round), this code is using lock 
files and not really using file-locking as intended.


I think the FileChannel.open usage is fine, I'm just not sure about the 
exception handling. For starters, FileSystemException is a super type of 
AccessDeniedException and NoSuchFileSystem so I don't think you need to 
catch them specifically. Would I be correct to say that the only reason 
that you would have to recover and try the next file is if the lock file 
exist? In that case then maybe it just needs to be:


try {
lockFileChannel = FileChannel.open(...);
} catch (FileAlreadyExistsException ignore) {
continue;
}

-Alan.

Re: RFR: 6244047: impossible to specify directories to logging FileHandler unless they exist

2012-11-13 Thread Jim Gish 

Here's a new webrev with my latest changes for your reviewing pleasure :-)

http://cr.openjdk.java.net/~jgish/Bug6244047-FileHandler-CheckLockLocation/ 
<http://cr.openjdk.java.net/%7Ejgish/Bug6244047-FileHandler-CheckLockLocation/>


Main changes:
- Using the file channel directly as suggested.
- Instead of checking up front for a valid directory I check the 
IOException on the channel open and process it accordingly. (BTW, I much 
prefer my previous proposed fix because I like to ensure pre-conditions 
for an operation are met prior to it rather than attempting the op, 
failing, and then recovering),
- Eliminated the stream, which really isn't needed, and just use the 
file channel


Just for the purposes of my enlightenment, assuming this is what you 
were after (Jason & Alan), what was your issue with checking for a valid 
directory up-front?


Thanks,
   Jim

On 11/13/2012 03:52 PM, Jim Gish wrote:


On 11/13/2012 03:36 PM, Jason Mehrens wrote:

Jim,

Looking at the webrev again I think I tricked myself into thinking 
that 'parentFile' was a file that could be opened with a stream when 
it actually is a directory.  I think the best fix would be to add a 
check in the catch block (around line 432) and only continue if the 
directory of the generated file 
java.nio.file.Files.isWritable/isDirectory otherwise throw the 
original exception.
I have a variant, which I think may do the trick which I'll post 
shortly. The catch block on 432 is for when the tryLock fails.  I 
think it's best to check the IOException around 420.


If the running JVM terminates abnormally won't the lock file fail to 
be deleted?  On restart, the lock file will exist to protect a dead 
process.
Yes, except that you can't really distinguish between that and another 
JVM using the lock file.  It's safer just to grab another file -- 
which is what the logic is already setup to do.


Jason



Date: Tue, 13 Nov 2012 13:25:27 -0500
From: jim.g...@oracle.com
To: alan.bate...@oracle.com
CC: jason_mehr...@hotmail.com; core-libs-dev@openjdk.java.net
Subject: Re: RFR: 6244047: impossible to specify directories to 
logging FileHandler unless they exist



On 11/13/2012 07:08 AM, Alan Bateman wrote:

On 12/11/2012 23:22, Jim Gish wrote:

Which file(s) are you concerned about truncating/damaging? 
The code I'm impacting is for creating a new lock file.  Where

is the potential for truncating/damaging that you both are
referring to?

Is this sufficient (plus the proper exception handling of
course) ?

//lockStream = new
FileOutputStream(lockFileName);
fc = FileChannel.open(new
File(lockFileName).toPath(), CREATE_NEW, WRITE);
//fc = lockStream.getChannel();

CREATE rather than CREATE_NEW so that it doesn't fail if the lock
file already exists. Although it's just a lock file then I think
it would be impolite to truncate it.

You could use Paths.get(lockFileName)rather than new
File(lockFileName).toPath() here but either is fine.

-Alan.

I think we want it to fail if the lock file already exists. That's 
why I used CREATE_NEW.  At least the way the logic is now, is that it 
attempts to create a new file and if it fails it tries again until it 
can create a new one.  It may be the case that the lockFileName is 
not in the locks map, and thus we don't own it, but it's possible 
that another JVM/app is logging in the same location.  It, of course, 
would be bad practice, but not disallowed.  We shouldn't be grabbing 
a lock file that might otherwise be in use.


Jim
--
Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304
Oracle Java Platform Group | Core Libraries Team
35 Network Drive
Burlington, MA 01803
jim.g...@oracle.com  <mailto:jim.g...@oracle.com>




--
Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304
Oracle Java Platform Group | Core Libraries Team
35 Network Drive
Burlington, MA 01803
jim.g...@oracle.com

Re: RFR: 6244047: impossible to specify directories to logging FileHandler unless they exist

2012-11-13 Thread Jim Gish 


On 11/13/2012 03:36 PM, Jason Mehrens wrote:

Jim,

Looking at the webrev again I think I tricked myself into thinking 
that 'parentFile' was a file that could be opened with a stream when 
it actually is a directory.  I think the best fix would be to add 
a check in the catch block (around line 432) and only continue if the 
directory of the generated 
file java.nio.file.Files.isWritable/isDirectory otherwise throw the 
original exception.
I have a variant, which I think may do the trick which I'll post 
shortly. The catch block on 432 is for when the tryLock fails.  I think 
it's best to check the IOException around 420.


If the running JVM terminates abnormally won't the lock file fail to 
be deleted?  On restart, the lock file will exist to protect a dead 
process.
Yes, except that you can't really distinguish between that and another 
JVM using the lock file.  It's safer just to grab another file -- which 
is what the logic is already setup to do.


Jason



Date: Tue, 13 Nov 2012 13:25:27 -0500
From: jim.g...@oracle.com
To: alan.bate...@oracle.com
CC: jason_mehr...@hotmail.com; core-libs-dev@openjdk.java.net
Subject: Re: RFR: 6244047: impossible to specify directories to 
logging FileHandler unless they exist



On 11/13/2012 07:08 AM, Alan Bateman wrote:

On 12/11/2012 23:22, Jim Gish wrote:

Which file(s) are you concerned about truncating/damaging? 
The code I'm impacting is for creating a new lock file.  Where

is the potential for truncating/damaging that you both are
referring to?

Is this sufficient (plus the proper exception handling of
course) ?

//lockStream = new
FileOutputStream(lockFileName);
fc = FileChannel.open(new
File(lockFileName).toPath(), CREATE_NEW, WRITE);
//fc = lockStream.getChannel();

CREATE rather than CREATE_NEW so that it doesn't fail if the lock
file already exists. Although it's just a lock file then I think
it would be impolite to truncate it.

You could use Paths.get(lockFileName)rather than new
File(lockFileName).toPath() here but either is fine.

-Alan.

I think we want it to fail if the lock file already exists. That's why 
I used CREATE_NEW.  At least the way the logic is now, is that it 
attempts to create a new file and if it fails it tries again until it 
can create a new one.  It may be the case that the lockFileName is not 
in the locks map, and thus we don't own it, but it's possible that 
another JVM/app is logging in the same location.  It, of course, would 
be bad practice, but not disallowed.  We shouldn't be grabbing a lock 
file that might otherwise be in use.


Jim
--
Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304
Oracle Java Platform Group | Core Libraries Team
35 Network Drive
Burlington, MA 01803
jim.g...@oracle.com  <mailto:jim.g...@oracle.com>


--
Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304
Oracle Java Platform Group | Core Libraries Team
35 Network Drive
Burlington, MA 01803
jim.g...@oracle.com

RE: RFR: 6244047: impossible to specify directories to logging FileHandler unless they exist

2012-11-13 Thread Jason Mehrens 

Jim,
 
Looking at the webrev again I think I tricked myself into thinking that 
'parentFile' was a file that could be opened with a stream when it actually is 
a directory.  I think the best fix would be to add a check in the catch block 
(around line 432) and only continue if the directory of the generated file 
java.nio.file.Files.isWritable/isDirectory otherwise throw the original 
exception.
 
If the running JVM terminates abnormally won't the lock file fail to be 
deleted?  On restart, the lock file will exist to protect a dead process.
 
Jason
 
 



Date: Tue, 13 Nov 2012 13:25:27 -0500
From: jim.g...@oracle.com
To: alan.bate...@oracle.com
CC: jason_mehr...@hotmail.com; core-libs-dev@openjdk.java.net
Subject: Re: RFR: 6244047: impossible to specify directories to logging 
FileHandler unless they exist



On 11/13/2012 07:08 AM, Alan Bateman wrote:

On 12/11/2012 23:22, Jim Gish wrote: 
Which file(s) are you concerned about truncating/damaging?  The code I'm 
impacting is for creating a new lock file.  Where is the potential for 
truncating/damaging that you both are referring to?

Is this sufficient (plus the proper exception handling of course) ?

//lockStream = new FileOutputStream(lockFileName);
fc = FileChannel.open(new File(lockFileName).toPath(), 
CREATE_NEW, WRITE);
//fc = lockStream.getChannel();
CREATE rather than CREATE_NEW so that it doesn't fail if the lock file already 
exists. Although it's just a lock file then I think it would be impolite to 
truncate it.

You could use Paths.get(lockFileName)rather than new 
File(lockFileName).toPath() here but either is fine.

-Alan.
I think we want it to fail if the lock file already exists.  That's why I used 
CREATE_NEW.  At least the way the logic is now, is that it attempts to create a 
new file and if it fails it tries again until it can create a new one.  It may 
be the case that the lockFileName is not in the locks map, and thus we don't 
own it, but it's possible that another JVM/app is logging in the same location. 
 It, of course, would be bad practice, but not disallowed.  We shouldn't be 
grabbing a lock file that might otherwise be in use.

Jim
-- 
Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304
Oracle Java Platform Group | Core Libraries Team
35 Network Drive
Burlington, MA 01803
jim.g...@oracle.com

Re: RFR: 6244047: impossible to specify directories to logging FileHandler unless they exist

2012-11-13 Thread Jim Gish 


On 11/13/2012 07:08 AM, Alan Bateman wrote:

On 12/11/2012 23:22, Jim Gish wrote:
Which file(s) are you concerned about truncating/damaging?  The code 
I'm impacting is for creating a new lock file.  Where is the 
potential for truncating/damaging that you both are referring to?


Is this sufficient (plus the proper exception handling of course) ?

//lockStream = new FileOutputStream(lockFileName);
fc = FileChannel.open(new 
File(lockFileName).toPath(), CREATE_NEW, WRITE);

//fc = lockStream.getChannel();
CREATE rather than CREATE_NEW so that it doesn't fail if the lock file 
already exists. Although it's just a lock file then I think it would 
be impolite to truncate it.


You could use Paths.get(lockFileName)rather than new 
File(lockFileName).toPath() here but either is fine.


-Alan.
I think we want it to fail if the lock file already exists.  That's why 
I used CREATE_NEW.  At least the way the logic is now, is that it 
attempts to create a new file and if it fails it tries again until it 
can create a new one.  It may be the case that the lockFileName is not 
in the locks map, and thus we don't own it, but it's possible that 
another JVM/app is logging in the same location. It, of course, would be 
bad practice, but not disallowed.  We shouldn't be grabbing a lock file 
that might otherwise be in use.


Jim

--
Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304
Oracle Java Platform Group | Core Libraries Team
35 Network Drive
Burlington, MA 01803
jim.g...@oracle.com

Re: RFR: 6244047: impossible to specify directories to logging FileHandler unless they exist

2012-11-13 Thread Alan Bateman 

On 12/11/2012 23:22, Jim Gish wrote:
Which file(s) are you concerned about truncating/damaging?  The code 
I'm impacting is for creating a new lock file.  Where is the potential 
for truncating/damaging that you both are referring to?


Is this sufficient (plus the proper exception handling of course) ?

//lockStream = new FileOutputStream(lockFileName);
fc = FileChannel.open(new 
File(lockFileName).toPath(), CREATE_NEW, WRITE);

//fc = lockStream.getChannel();
CREATE rather than CREATE_NEW so that it doesn't fail if the lock file 
already exists. Although it's just a lock file then I think it would be 
impolite to truncate it.


You could use Paths.get(lockFileName)rather than new 
File(lockFileName).toPath() here but either is fine.


-Alan.

Re: RFR: 6244047: impossible to specify directories to logging FileHandler unless they exist

2012-11-12 Thread Jim Gish 
Which file(s) are you concerned about truncating/damaging?  The code I'm 
impacting is for creating a new lock file.  Where is the potential for 
truncating/damaging that you both are referring to?


Is this sufficient (plus the proper exception handling of course) ?

//lockStream = new FileOutputStream(lockFileName);
fc = FileChannel.open(new 
File(lockFileName).toPath(), CREATE_NEW, WRITE);

//fc = lockStream.getChannel();

Thanks,
   Jim

On 11/10/2012 05:54 AM, Alan Bateman wrote:

On 09/11/2012 22:41, Jason Mehrens wrote:

Jim,

You might just want to change the code to create and close a 
FileOutputStream in a way that doesn't truncate or damage the target 
file.  Or maybe use the NIO file code if that is possible.  See BUG 
ID 4420020.


Jason
I think so too. As it needs a FileChannel anyway, then it may be 
simpler to just use FileChannel.open(lf, WRITE), that won't truncate 
the file and will also throw a useful IOException in the event that it 
fails. As there are specific IOException thrown for specific cases 
then it may be possible to eliminate the loop completely for I/O error 
cases.


-Alan


--
Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304
Oracle Java Platform Group | Core Libraries Team
35 Network Drive
Burlington, MA 01803
jim.g...@oracle.com

Re: RFR: 6244047: impossible to specify directories to logging FileHandler unless they exist

2012-11-10 Thread Alan Bateman 

On 09/11/2012 22:41, Jason Mehrens wrote:

Jim,

You might just want to change the code to create and close a FileOutputStream 
in a way that doesn't truncate or damage the target file.  Or maybe use the NIO 
file code if that is possible.  See BUG ID 4420020.

Jason
I think so too. As it needs a FileChannel anyway, then it may be simpler 
to just use FileChannel.open(lf, WRITE), that won't truncate the file 
and will also throw a useful IOException in the event that it fails. As 
there are specific IOException thrown for specific cases then it may be 
possible to eliminate the loop completely for I/O error cases.


-Alan

RE: RFR: 6244047: impossible to specify directories to logging FileHandler unless they exist

2012-11-09 Thread Jason Mehrens 

Jim,
 
You might just want to change the code to create and close a FileOutputStream 
in a way that doesn't truncate or damage the target file.  Or maybe use the NIO 
file code if that is possible.  See BUG ID 4420020.
 
Jason
 

> Date: Fri, 9 Nov 2012 16:37:02 -0500
> From: jim.g...@oracle.com
> To: core-libs-dev@openjdk.java.net
> Subject: RFR: 6244047: impossible to specify directories to logging 
> FileHandler unless they exist
> 
> Please review 
> http://cr.openjdk.java.net/~jgish/Bug6244047-FileHandler-CheckLockLocation/ 
> <http://cr.openjdk.java.net/%7Ejgish/Bug6244047-FileHandler-CheckLockLocation/>
> 
> This updates the logging FileHandler to actually check the directory 
> passed to it via the pattern to ensure that it exists and is writable. 
> It does this before going into the loop to create lock files there which 
> will fail repeatedly if the directory specified is invalid. If the file 
> specified does not exist, or is not a directory or not writable, an 
> IOException with a precise message is thrown.
> 
> Note that this fix does not do as some users would like, which is to go 
> ahead and create directories that don't exist.
> 
> Thanks,
> Jim
> 
> -- 
> Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304
> Oracle Java Platform Group | Core Libraries Team
> 35 Network Drive
> Burlington, MA 01803
> jim.g...@oracle.com
>

RFR: 6244047: impossible to specify directories to logging FileHandler unless they exist

2012-11-09 Thread Jim Gish 
Please review 
http://cr.openjdk.java.net/~jgish/Bug6244047-FileHandler-CheckLockLocation/ 



This updates the logging FileHandler to actually check the directory 
passed to it via the pattern to ensure that it exists and is writable.  
It does this before going into the loop to create lock files there which 
will fail repeatedly if the directory specified is invalid.  If the file 
specified does not exist, or is not a directory or not writable, an 
IOException with a precise message is thrown.


Note that this fix does not do as some users would like, which is to go 
ahead and create directories that don't exist.


Thanks,
   Jim

--
Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304
Oracle Java Platform Group | Core Libraries Team
35 Network Drive
Burlington, MA 01803
jim.g...@oracle.com