From 1484d6eb0ac36dc8e31f5069b77e4ecfd573ad17 Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Thu, 26 Apr 2018 15:14:50 -0700 Subject: [PATCH] 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 --- .../storage/dfs/LargePackedWholeObject.java | 31 ++++++++++++------- .../internal/storage/dfs/PackInputStream.java | 2 +- 2 files changed, 20 insertions(+), 13 deletions(-) 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 b6e9d319aa..343dc11ae3 100644 --- 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 d88011c3ae..b859d9df33 100644 --- 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; -- 2.39.5