8283335 : Add exists and readAttributesIfExists methods to FileSystemProvider #9249
Conversation
|
👋 Welcome back lancea! A progress list of the required criteria for merging this PR into |
|
@LanceAndersen The following labels will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
| * are followed. If the option {@link LinkOption#NOFOLLOW_LINKS NOFOLLOW_LINKS} | ||
| * is present then symbolic links are not followed and the method | ||
| * {@link #readAttributes(Path, Class, LinkOption...)} is called | ||
| * to determine whether a file exists. |
There was a problem hiding this comment.
Indentation is off at lines 1183-1184.
There was a problem hiding this comment.
Thanks for catching that. Will be addressed in the next push
| // get the UnixFileAttributes for a given file. Returns null if the file does not exist. | ||
| static UnixFileAttributes getIfExists(UnixPath path) throws UnixException { | ||
| UnixFileAttributes attrs = new UnixFileAttributes(); | ||
| int errno = UnixNativeDispatcher.stat2(path, attrs); |
There was a problem hiding this comment.
I think that this is the only use of stat2(). It could be deleted if this method were replaced with this:
static UnixFileAttributes getIfExists(UnixPath path) throws UnixException {
UnixFileAttributes attrs = new UnixFileAttributes();
try {
UnixNativeDispatcher.stat(path, attrs);
return attrs;
} catch (UnixException e) {
if (e.errno() == UnixConstants.ENOENT) {
return null;
}
throw e;
}
}
There was a problem hiding this comment.
I think that this is the only use of
stat2(). It could be deleted if this method were replaced with this:
The purpose of these additions to the SPI is to improve the performance of user facing methods that don't throw an exception when the file doesn't exist. It needs a stat/equivalent that doesn't throw so this is the reason for stat2. The equivalent on Windows will need to added too.
| * <p> It is implementation specific if all file attributes are read as an | ||
| * atomic operation with respect to other file system operations. | ||
| * | ||
| * @implSpec |
There was a problem hiding this comment.
fixed in next push
| * @since 20 | ||
| */ | ||
| public boolean exists(Path path, LinkOption... options) { | ||
| try { |
There was a problem hiding this comment.
Overall, I think that this is a great change (avoiding the need for various parts of the system to communicate through exceptions).
Trivially, and easily, I often find it useful to add mock-like tests for these specified implementations (implSpec), i.e. just subclass FSP mocking out the abstract methods to ensure that the default implementations invoke the appropriate ones.
There was a problem hiding this comment.
Thank you for the suggestion Chris
|
This should be performance neutral for the default provider and positive for the zip provider. Do we have any micros that could be added? |
| case READ -> r = true; | ||
| case WRITE -> w = true; | ||
| case EXECUTE -> x = true; | ||
| default -> throw new AssertionError("Should not get here"); |
There was a problem hiding this comment.
The switch should be exhaustive so I assume the default case is not required here.
There was a problem hiding this comment.
Addressed in next push
|
The end date in the copyright header will need updated a few files, I assume you'll do that before this change is integrated. |
| LinkOption... options) | ||
| throws IOException | ||
| { | ||
| return exists(path) ? readAttributes(path, type, options) : null; |
There was a problem hiding this comment.
If you add the following to ZipPath then it would allow you to implement it in one step rather than two:
ZipFileAttributes readAttributesIfExits() throws IOException {
return zfs.getFileAttributes(getResolvedPath());
}
There was a problem hiding this comment.
Added the proposed change above, Mach5 tiers 1-3 continue to be clean
|
The implementation changes in latest version address my previous points, thanks! I don't the test as it duplicates most of the existing of the TestProvider in this directory and the naming is inconsistent with the existing tests. I expect you should be able to just extent TestProvider and override the two methods to record that they can been called. |
I thought about using TestProvider and then talked myself out of it. So yes, I can do that. If you have a preferred name other than |
The only change that should be needed to TestProvider is to implement the checkAccess method with: With that change it should be easy to extend and override exists/readAttributesIfExists to record that they are called. The need to run with the test with -Djava.nio.file.spi.DefaultFileSystemProvider=TestProvider goes away too and and should be much simpler to test that the expected delegation. |
|
The updated TestDelegation test is looking a bit better now but I think it would be simplified a lot more by getting rid of the data providers, just aren't needed in this test. |
Unless you feel this is a must, I would prefer to keep the DataProviders. The benefit I see is the test code does not need to be duplicated per parameter, each test scenario can be run as an individual test so that you do not need extra plumbing to run each test scenario in the unlikely event of a failure. |
Okay, but there are a few other things to mention: One issue is the reset method is called at the end of each test. I think it needs to be at the beginning of the method, moved to a finally block of a try-finally, or maybe @BeforeMethod to reset before each test. The reason is that one test failing will cause the tests that follow to fail too. The fields aren't constants so looks a bit strange (to me anyway) to be in uppercase. If you rename them then I think the tests would be a bit more readable. |
Good catch, added
Ah, sometimes you see what you want. I must have had fields on my mind when I did that as I completely agree with you. Both of the above are addressed in the latest update to the PR. Thank you for the feedback. |
|
@LanceAndersen This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 150 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
|
/integrate |
|
Going to push as commit d48694d.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@LanceAndersen Pushed as commit d48694d. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Hi,
Please review the following patch which will:
Enhance the java.nio.file.spi.FileSystemProvider abstract class to include the methods
This change allows for providers to provide optimizations when the file's attributes are not needed.
Mach5 tiers 1 - 3 run clean with this change
The CSR may be viewed at JDK-8283336
Best,
Lance
Progress
Issues
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9249/head:pull/9249$ git checkout pull/9249Update a local copy of the PR:
$ git checkout pull/9249$ git pull https://git.openjdk.org/jdk pull/9249/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 9249View PR using the GUI difftool:
$ git pr show -t 9249Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9249.diff