On Tue, 15 Oct 2024 22:37:26 GMT, Brian Burkhalter <b...@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
