On Wed, 5 May 2021 23:23:21 GMT, Jason Zaugg <jza...@openjdk.org> wrote:
>> 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.
I've added the simplifying commit for now, but I'm happy to split that to a
separate change if you prefer.
-------------
PR: https://git.openjdk.java.net/jdk/pull/3853
