Browse Source

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.

b0ac5f9c89 (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
stable-5.0
Jonathan Nieder 7 years ago
parent
commit
1484d6eb0a
  1. 21
      org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/LargePackedWholeObject.java
  2. 2
      org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/PackInputStream.java

21
org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/LargePackedWholeObject.java

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

2
org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/PackInputStream.java

@ -47,7 +47,7 @@ import java.io.IOException;
import java.io.InputStream; import java.io.InputStream;
final class PackInputStream extends InputStream { final class PackInputStream extends InputStream {
private final DfsReader ctx; final DfsReader ctx;
private final DfsPackFile pack; private final DfsPackFile pack;

Loading…
Cancel
Save