RFR: 8233451: (fs) Files.newInputStream() cannot be used with character special files [v2]

[Alan Bateman]

On Tue, 15 Oct 2024 22:37:26 GMT, Brian Burkhalter <bpb at openjdk.org> wrote:

>> Add `isOther` and `available` methods to `FileChannelImpl` and the interfaces to native code and use these in `ChannelInputStream` to work around cases where a wrapped `FileChannelImpl` is not really seekable.
>
> Brian Burkhalter has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - 8233451: Remove use of handleAvailable() (Windows)
>  - 8233451: Remove use of handleAvailable() (UNIX)

src/java.base/share/classes/sun/nio/ch/ChannelInputStream.java line 56:

> 54:     private byte[] b1;
> 55: 
> 56:     private Boolean isOther = null;

I don't particularly like using a Boolean for tri-states but it's not too bad here. No need to initialize to null. It could be Stable but probably not much benefit here all usages require file I/O that dominates.

Are you going to add a comment to this field as readers might know now what "other" means? In the APIs we say "something other than a regular file, directory, or symbolic link" and maybe that could be useful here.

src/java.base/share/classes/sun/nio/ch/FileChannelImpl.java line 533:

> 531:     }
> 532: 
> 533:     public int available() throws IOException {

This can be package-private. It would be useful to add a method description as FC doesn't define this method, same thing for isOther.

src/java.base/share/classes/sun/nio/ch/FileChannelImpl.java line 565:

> 563:                 return isOther = nd.isOther(fd);
> 564:             } finally {
> 565:                 Blocker.end(attempted);

No need for Blocker begin/end here, we only use for direct I/O or file locking.

src/java.base/unix/native/libnio/ch/UnixFileDispatcherImpl.c line 233:

> 231: 
> 232:     return JNI_TRUE;
> 233: }

Can you check these syscalls for EINTR? Might be okay, but worth checking.

src/java.base/windows/native/libnio/ch/FileDispatcherImpl.c line 438:

> 436: 
> 437:     BY_HANDLE_FILE_INFORMATION finfo;
> 438:     GetFileInformationByHandle(handle, &finfo);

This returns a BOOL that needs to be checked.

test/jdk/java/nio/file/Files/InputStreamTest.java line 139:

> 137:             InputStream s = Files.newInputStream(stdin);
> 138:             s.available();
> 139:         }

I assume you meant to close the input stream.

What about the other methods defined by InputStream that have special handling in ChannelInputStream? I assume we should add test coverage for these methods.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/21508#discussion_r1804577608
PR Review Comment: https://git.openjdk.org/jdk/pull/21508#discussion_r1804580164
PR Review Comment: https://git.openjdk.org/jdk/pull/21508#discussion_r1804581541
PR Review Comment: https://git.openjdk.org/jdk/pull/21508#discussion_r1804585370
PR Review Comment: https://git.openjdk.org/jdk/pull/21508#discussion_r1804586710
PR Review Comment: https://git.openjdk.org/jdk/pull/21508#discussion_r1804588059