8365807: (fs) Two-arg UnixFileAttributes.getIfExists should not use exception for control flow

[bplb]

In the Unix implementation of sun.nio.fs, change

1. UnixFileAttributes.getIfExists(UnixPath,boolean) to use the system call success status and errno for control flow; and
2. UnixFileAttributes.getIfExists(UnixPath) to simply return UnixFileAttributes.getIfExists(UnixPath,true).

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8365807: (fs) Two-arg UnixFileAttributes.getIfExists should not use exception for control flow (Bug - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26889/head:pull/26889
$ git checkout pull/26889

Update a local copy of the PR:
$ git checkout pull/26889
$ git pull https://git.openjdk.org/jdk.git pull/26889/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 26889

View PR using the GUI difftool:
$ git pr show -t 26889

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26889.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back bpb! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

@bplb The following label will be automatically applied to this pull request:

• nio

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 01: Full - Incremental (7ea1b084)
• 00: Full (a4545b9f)

[AlanBateman]

If dfd/path does not exist then won't this now return an unpopulated UnixFileAttributes?

I think my concern is that the changes create a bit of a hazard. Most UnixNativeDispatcher native methods throw if the syscall fails, a small number return the errno, now we have a native method that special cases ENOENT. I think we should try to limit it to two variants, one that throws, the other that returns errno, and establish some naming convention to reduce the possibility of mis-use. It would be good to audit out tests to make sure that every method exercises error cases.

[bplb]

This change was intended to address this comment

a version that doesn't use exceptions for control flow,

but I think I misinterpreted that. I'll reassess the change.

[bplb]

Commit 7ea1b08:

1. Change fstatat0 and stat0 to throw instead of returning errno.
2. Remove the stat2 method.
3. Change calling code to handle the changes.

Now all ?stat{at}0 functions are void, expressing errors via UnixException.

All UnixNativeDispatcher methods are now void except:

jbytearray getcwd, getgrid, getgrnam, getpwnam, getpwuid, readdir, realpath, strerror
jint       access, dup, fgetxattr, flistxattr, getlinelen, openat, read, write
jlong      fdopendir, opendir

All these functions return a "useful" value, except access which returns errno. It would be a little disruptive to change access to throw instead of returning errno as there are a number of usages like

return access(path, mode) == 0;

[AlanBateman]

UnixFileAttributes.getIfExists is performance critical so this is why it the native method returns errno rather than throws. The suggestion is the new 2-arg getIfExits work the same way and only throw if the error is not ENOENT. My comment on void vs. int return is to point out the hazard when we have two styles, it's too easy to call the method that does not throw when an exception is expected. This lead to the suggestion to choose a naming convention to avoid mistakes.

[bplb]

UnixFileAttributes.getIfExists is performance critical so this is why it the native method returns errno rather than throws.

So the stat0 and fstatat0 methods should revert to returning errno.

[bplb]

This lead to the suggestion to choose a naming convention to avoid mistakes.

As there are only a couple of methods that return errno, then maybe something like fstatat0NoThrow or fstatat0errno and leave the ones that throw as they are?