On Thu, 6 May 2021 08:05:12 GMT, Jason Zaugg <jza...@openjdk.org> wrote:
>> src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 1235:
>>
>>> 1233: synchronized(ch) {
>>> 1234: return ch.position(pos).read(bb);
>>> 1235: }
>>
>> I think it's okay to include the update to EntryInputStream, that part looks
>> fine, as does the directly use of the FileChannel positional read.
>>
>> I'm still mulling over the case where ch is not a FileChannel as I would
>> expected it to capture the existing position and restore it after the read.
>> I think this is the degenerative case when the zip file is located in a
>> custom file system that doesn't support FileChannel. In that case,
>> positional read has to be implemented on the most basic SeekableByteChannel.
>> It would only be observed when mixing positional read ops with other ops
>> that depend on the current position.
>
> Here are all the references to `ch`.
>
>
> this.ch = Files.newByteChannel(zfpath, READ);
> ...
> this.ch.close();
> ...
> ch.close(); // close the ch just in case no update
> ...
> if (ch instanceof FileChannel fch) {
> return fch.read(bb, pos);
> } else {
> synchronized(ch) {
> return ch.position(pos).read(bb);
> }
> }
> ...
> long ziplen = ch.size();
> ...
> ch.close();
>
>
> It appears the only position-dependent operation called `read(ByteBuffer)`.
> This is performed together with the `pos` call within the `synchronized(ch)`
> lock.
I have confirmed that the non-`FileChannel` code path is exercised by existing
tests.
test/jdk/jdk/nio/zipfs/ZipFSTester.java includes a test that forms a file
system based on a JAR that is itself an entry within another `ZipFileSystem`.
Sample stacks:
java.lang.Throwable: readFullyAt. ch.getClass=class
jdk.nio.zipfs.ByteArrayChannel
at
jdk.zipfs/jdk.nio.zipfs.ZipFileSystem.readFullyAt(ZipFileSystem.java:1234)
at
jdk.zipfs/jdk.nio.zipfs.ZipFileSystem.readFullyAt(ZipFileSystem.java:1226)
at
jdk.zipfs/jdk.nio.zipfs.ZipFileSystem$EntryInputStream.initDataPos(ZipFileSystem.java:2259)
at
jdk.zipfs/jdk.nio.zipfs.ZipFileSystem$EntryInputStream.read(ZipFileSystem.java:2201)
at jdk.zipfs/jdk.nio.zipfs.ZipFileSystem$2.fill(ZipFileSystem.java:2151)
at
java.base/java.util.zip.InflaterInputStream.read(InflaterInputStream.java:158)
at ZipFSTester.checkEqual(ZipFSTester.java:858)
at ZipFSTester.test1(ZipFSTester.java:259)
java.lang.Throwable: readFullyAt. ch.getClass=class
jdk.nio.zipfs.ByteArrayChannel
at
jdk.zipfs/jdk.nio.zipfs.ZipFileSystem.readFullyAt(ZipFileSystem.java:1234)
at
jdk.zipfs/jdk.nio.zipfs.ZipFileSystem$EntryInputStream.read(ZipFileSystem.java:2214)
at jdk.zipfs/jdk.nio.zipfs.ZipFileSystem$2.fill(ZipFileSystem.java:2151)
at
java.base/java.util.zip.InflaterInputStream.read(InflaterInputStream.java:158)
at ZipFSTester.checkEqual(ZipFSTester.java:858)
at ZipFSTester.test1(ZipFSTester.java:259)
This use case is not covered by the `ZipFSTester.test2`, a multi-threaded test.
While looking at the test I noticed false warnings in the output:
`read()/position() failed`. This did not actually fail the test. I investigated
this and a) fixed the condition to deal with the edge case of zero-length
entries and b) throw an "check failed" exception when the assertion fails.
-------------
PR: https://git.openjdk.java.net/jdk/pull/3853
