LargePackedWholeObject: Do not reuse released inflater

LargePackedWholeObject.openStream produces a stream that allows
reading a large object.  This stream holds a DfsReader that takes care
of caching delta bases etc and in particular holds zlib Inflater for
use while reading the each delta in the packfile.

At DfsReader creation time, the Inflater is acquired from a global
InflaterCache to avoid initialization overhead in case there is an
existing Inflater available for reuse.  When done with the Inflater,
the DfsReader is responsible for returning it to the cache for reuse.
The DfsReader is AutoClosable to remind the caller to close it and
release the Inflater when finished with it.

b0ac5f9c8907a4034612543a92eb465e88a9c6f2 (LargePackedWholeObject:
Refactor to open DfsReader in try-with-resource, 2018-04-11) tried to
clarify the lifetime of the DfsReader but was too aggressive: when
this function returns, PackInputStream owns the DfsReader and is
already going to release it.  Worse, the returned InflaterInputStream
holds a reference to the DfsReader's inflater, making releasing the
DfsReader not only unnecessary but unsafe.

The Inflater gets released into the InflaterCache's pool, to be
acquired by another caller that uses it concurrently with the
InflaterInputStream.  This results in errors, such as

 java.util.zip.ZipException: incorrect header check
        at java.util.zip.InflaterInputStream.read(InflaterInputStream.java:164)
        at java.util.zip.InflaterInputStream.skip(InflaterInputStream.java:208)
        at java.io.BufferedInputStream.skip(BufferedInputStream.java:377)

and

 java.util.zip.DataFormatException: incorrect header check
        at java.util.zip.Inflater.inflateBytes(Native Method)
        at java.util.zip.Inflater.inflate(Inflater.java:259)
        at org.eclipse.jgit.internal.storage.dfs.DfsReader.inflate(DfsReader.java:783)
        at org.eclipse.jgit.internal.storage.dfs.DfsPackFile.decompress(DfsPackFile.java:420)
        at org.eclipse.jgit.internal.storage.dfs.DfsPackFile.load(DfsPackFile.java:767)

and

 Caused by: java.util.zip.ZipException: incorrect header check
        at java.util.zip.InflaterInputStream.read(InflaterInputStream.java:164)
        at java.io.BufferedInputStream.fill(BufferedInputStream.java:246)
        at java.io.BufferedInputStream.read1(BufferedInputStream.java:286)
        at java.io.BufferedInputStream.read(BufferedInputStream.java:345)
        at org.eclipse.jgit.lib.ObjectStream$Filter.read(ObjectStream.java:219)
        at org.eclipse.jgit.util.IO.readFully(IO.java:233)
        at org.eclipse.jgit.transport.PackParser.checkObjectCollision(PackParser.java:1173)

Verified in production.  It should be possible to make a
straightforward unit test for this using the InflaterCache state but
that can wait for a followup commit.

Change-Id: Iaf1d6fd368b64f76c520d215fd270a6098a1f236

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/LargePackedWholeObject.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/LargePackedWholeObject.java
index b6e9d319aa379cc9b9bf2526fb210455b42217b5..343dc11ae3733da505980bbb69bd3a6bae003559 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/LargePackedWholeObject.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/LargePackedWholeObject.java
@@ -104,26 +104,33 @@ final class LargePackedWholeObject extends ObjectLoader {
        /** {@inheritDoc} */
        @Override
        public ObjectStream openStream() throws MissingObjectException, IOException {
-               InputStream in;
-               try (DfsReader ctx = db.newReader()) {
+               PackInputStream packIn;
+               DfsReader ctx = db.newReader();
+               try {
                        try {
-                               in = new PackInputStream(pack, objectOffset + headerLength, ctx);
+                               packIn = new PackInputStream(
+                                               pack, objectOffset + headerLength, ctx);
+                               ctx = null; // owned by packIn
                        } catch (IOException packGone) {
                                // If the pack file cannot be pinned into the cursor, it
                                // probably was repacked recently. Go find the object
                                // again and open the stream from that location instead.
-                               //
                                ObjectId obj = pack.getReverseIdx(ctx).findObject(objectOffset);
                                return ctx.open(obj, type).openStream();
                        }
-
-                       // Align buffer to inflater size, at a larger than default block.
-                       // This reduces the number of context switches from the
-                       // caller down into the pack stream inflation.
-                       int bufsz = 8192;
-                       in = new BufferedInputStream(
-                                       new InflaterInputStream(in, ctx.inflater(), bufsz), bufsz);
-                       return new ObjectStream.Filter(type, size, in);
+               } finally {
+                       if (ctx != null) {
+                               ctx.close();
+                       }
                }
+
+               // Align buffer to inflater size, at a larger than default block.
+               // This reduces the number of context switches from the
+               // caller down into the pack stream inflation.
+               int bufsz = 8192;
+               InputStream in = new BufferedInputStream(
+                               new InflaterInputStream(packIn, packIn.ctx.inflater(), bufsz),
+                               bufsz);
+               return new ObjectStream.Filter(type, size, in);
        }
 }

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/PackInputStream.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/PackInputStream.java
index d88011c3ae40fbda6f1e8fdb689712472db17c8f..b859d9df333073503093cb3a4acc2ac2749b229c 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/PackInputStream.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/PackInputStream.java
@@ -47,7 +47,7 @@ import java.io.IOException;
 import java.io.InputStream;
 
 final class PackInputStream extends InputStream {
-       private final DfsReader ctx;
+       final DfsReader ctx;
 
        private final DfsPackFile pack;