On Wed, 5 May 2021 16:03:17 GMT, Alan Bateman <[email protected]> wrote:
>> Yes, I plan to look at this. It would also be good to have a couple of
>> additional reviews as well :-)
>
> I think using the positional read on the underlying FileChannel is okay. I'm
> puzzled by the previous code as I would have expected it to restore the
> position (make me wonder if there are zipfs tests for this).
My reading of the existing code is that the only position-influenced method
called on the channel (either via `ZipFileSystem.ch` or
`ZipFileSystem$EntryInputStream.zfch`) is `read`, and this is only called in
the `.position(pos).read(...)` idiom. The failure to reset the position doesn't
affect correctness. However the `synchronzized` _is_ definitely needed to avoid
races.
Incidentally, regarding this comment:
private class EntryInputStream extends InputStream {
private final SeekableByteChannel zfch; // local ref to zipfs's "ch".
zipfs.ch might
// point to a new channel after
sync()
If the file system is writable and updated, the underlying file is deleted and
replaced with a temporary file by `close()` / `sync()`, but `ZipFileSystem.ch`
is itself final since d581e4f4. I believe the comment is outdated and
`EntryInputStream` could just access ch via the outer pointer. That change
would simplify this patch marginally.
-------------
PR: https://git.openjdk.java.net/jdk/pull/3853
