Cleanup stream usage WRT filters
As it is right now some streams leak out of the filter construct. This
change clarifies responsibilities and fixes stream leaks
Change-Id: Ib9717d43a701a06a502434d64214d13a392de5ab
Signed-off-by: Markus Duft <markus.duft@ssi-schaefer.com>
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
diff --git a/org.eclipse.jgit.lfs/src/org/eclipse/jgit/lfs/CleanFilter.java b/org.eclipse.jgit.lfs/src/org/eclipse/jgit/lfs/CleanFilter.java
index fccbae7..217e46f 100644
--- a/org.eclipse.jgit.lfs/src/org/eclipse/jgit/lfs/CleanFilter.java
+++ b/org.eclipse.jgit.lfs/src/org/eclipse/jgit/lfs/CleanFilter.java
@@ -167,6 +167,7 @@ public int run() throws IOException {
}
LfsPointer lfsPointer = new LfsPointer(loid, size);
lfsPointer.encode(out);
+ in.close();
out.close();
return -1;
}
@@ -174,6 +175,7 @@ public int run() throws IOException {
if (aOut != null) {
aOut.abort();
}
+ in.close();
out.close();
throw e;
}
diff --git a/org.eclipse.jgit.lfs/src/org/eclipse/jgit/lfs/SmudgeFilter.java b/org.eclipse.jgit.lfs/src/org/eclipse/jgit/lfs/SmudgeFilter.java
index 142e74d..5c0d3b4 100644
--- a/org.eclipse.jgit.lfs/src/org/eclipse/jgit/lfs/SmudgeFilter.java
+++ b/org.eclipse.jgit.lfs/src/org/eclipse/jgit/lfs/SmudgeFilter.java
@@ -116,23 +116,29 @@ static void register() {
* @param db
* a {@link org.eclipse.jgit.lib.Repository} object.
* @param in
- * a {@link java.io.InputStream} object.
+ * a {@link java.io.InputStream} object. The stream is closed in
+ * any case.
* @param out
* a {@link java.io.OutputStream} object.
* @throws java.io.IOException
+ * in case of an error
*/
public SmudgeFilter(Repository db, InputStream in, OutputStream out)
throws IOException {
super(in, out);
- Lfs lfs = new Lfs(db);
- LfsPointer res = LfsPointer.parseLfsPointer(in);
- if (res != null) {
- AnyLongObjectId oid = res.getOid();
- Path mediaFile = lfs.getMediaFile(oid);
- if (!Files.exists(mediaFile)) {
- downloadLfsResource(lfs, db, res);
+ try {
+ Lfs lfs = new Lfs(db);
+ LfsPointer res = LfsPointer.parseLfsPointer(in);
+ if (res != null) {
+ AnyLongObjectId oid = res.getOid();
+ Path mediaFile = lfs.getMediaFile(oid);
+ if (!Files.exists(mediaFile)) {
+ downloadLfsResource(lfs, db, res);
+ }
+ this.in = Files.newInputStream(mediaFile);
}
- this.in = Files.newInputStream(mediaFile);
+ } finally {
+ in.close(); // make sure the swapped stream is closed properly.
}
}
@@ -147,6 +153,7 @@ public SmudgeFilter(Repository db, InputStream in, OutputStream out)
* the objects to download
* @return the paths of all mediafiles which have been downloaded
* @throws IOException
+ * @since 4.11
*/
public static Collection<Path> downloadLfsResource(Lfs lfs, Repository db,
LfsPointer... res) throws IOException {
@@ -228,33 +235,39 @@ public static Collection<Path> downloadLfsResource(Lfs lfs, Repository db,
/** {@inheritDoc} */
@Override
public int run() throws IOException {
- int totalRead = 0;
- int length = 0;
- if (in != null) {
- byte[] buf = new byte[8192];
- while ((length = in.read(buf)) != -1) {
- out.write(buf, 0, length);
- totalRead += length;
+ try {
+ int totalRead = 0;
+ int length = 0;
+ if (in != null) {
+ byte[] buf = new byte[8192];
+ while ((length = in.read(buf)) != -1) {
+ out.write(buf, 0, length);
+ totalRead += length;
- // when threshold reached, loop back to the caller. otherwise we
- // could only support files up to 2GB (int return type)
- // properly. we will be called again as long as we don't return
- // -1 here.
- if (totalRead >= MAX_COPY_BYTES) {
- // leave streams open - we need them in the next call.
- return totalRead;
+ // when threshold reached, loop back to the caller.
+ // otherwise we could only support files up to 2GB (int
+ // return type) properly. we will be called again as long as
+ // we don't return -1 here.
+ if (totalRead >= MAX_COPY_BYTES) {
+ // leave streams open - we need them in the next call.
+ return totalRead;
+ }
}
}
- }
- if (totalRead == 0 && length == -1) {
- // we're totally done :)
- in.close();
+ if (totalRead == 0 && length == -1) {
+ // we're totally done :) cleanup all streams
+ in.close();
+ out.close();
+ return length;
+ }
+
+ return totalRead;
+ } catch (IOException e) {
+ in.close(); // clean up - we swapped this stream.
out.close();
- return length;
+ throw e;
}
-
- return totalRead;
}
}
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/attributes/FilterCommand.java b/org.eclipse.jgit/src/org/eclipse/jgit/attributes/FilterCommand.java
index aae0076..c4357d1 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/attributes/FilterCommand.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/attributes/FilterCommand.java
@@ -67,6 +67,9 @@ public abstract class FilterCommand {
/**
* Constructor for FilterCommand
+ * <p>
+ * FilterCommand implementors are required to manage the in and out streams
+ * (close on success and/or exception).
*
* @param in
* The {@link java.io.InputStream} this command should read from
@@ -84,6 +87,9 @@ public FilterCommand(InputStream in, OutputStream out) {
* number of bytes it read from {@link #in}. It should be called in a loop
* until it returns -1 signaling that the {@link java.io.InputStream} is
* completely processed.
+ * <p>
+ * On successful completion (return -1) or on Exception, the streams
+ * {@link #in} and {@link #out} are closed by the implementation.
*
* @return the number of bytes read from the {@link java.io.InputStream} or
* -1. -1 means that the {@link java.io.InputStream} is completely
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/WorkingTreeIterator.java b/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/WorkingTreeIterator.java
index 4ebf01f..68cc7cb 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/WorkingTreeIterator.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/WorkingTreeIterator.java
@@ -476,7 +476,7 @@ private InputStream filterClean(InputStream in, OperationType opType)
while (command.run() != -1) {
// loop as long as command.run() tells there is work to do
}
- return buffer.openInputStream();
+ return buffer.openInputStreamWithAutoDestroy();
}
FS fs = repository.getFS();
ProcessBuilder filterProcessBuilder = fs.runInShell(filterCommand,
@@ -499,7 +499,7 @@ filterCommand, getEntryPathString(),
RawParseUtils.decode(result.getStderr()
.toByteArray(MAX_EXCEPTION_TEXT_SIZE))));
}
- return result.getStdout().openInputStream();
+ return result.getStdout().openInputStreamWithAutoDestroy();
}
return in;
}
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/util/LfsFactory.java b/org.eclipse.jgit/src/org/eclipse/jgit/util/LfsFactory.java
index 10fe564..9200916 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/util/LfsFactory.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/util/LfsFactory.java
@@ -259,7 +259,7 @@ public LfsInputStream(InputStream stream, long length) {
* in case of an error opening the stream to the buffer.
*/
public LfsInputStream(TemporaryBuffer buffer) throws IOException {
- this.stream = buffer.openInputStream();
+ this.stream = buffer.openInputStreamWithAutoDestroy();
this.length = buffer.length();
}
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/util/TemporaryBuffer.java b/org.eclipse.jgit/src/org/eclipse/jgit/util/TemporaryBuffer.java
index 31abb7c..dd933a0 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/util/TemporaryBuffer.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/util/TemporaryBuffer.java
@@ -314,6 +314,26 @@ public InputStream openInputStream() throws IOException {
}
/**
+ * Same as {@link #openInputStream()} but handling destruction of any
+ * associated resources automatically when closing the returned stream.
+ *
+ * @return an InputStream which will automatically destroy any associated
+ * temporary file on {@link #close()}
+ * @throws IOException
+ * in case of an error.
+ * @since 4.11
+ */
+ public InputStream openInputStreamWithAutoDestroy() throws IOException {
+ return new BlockInputStream() {
+ @Override
+ public void close() throws IOException {
+ super.close();
+ destroy();
+ }
+ };
+ }
+
+ /**
* Reset this buffer for reuse, purging all buffered content.
*/
public void reset() {
@@ -506,6 +526,20 @@ public InputStream openInputStream() throws IOException {
}
@Override
+ public InputStream openInputStreamWithAutoDestroy() throws IOException {
+ if (onDiskFile == null) {
+ return super.openInputStreamWithAutoDestroy();
+ }
+ return new FileInputStream(onDiskFile) {
+ @Override
+ public void close() throws IOException {
+ super.close();
+ destroy();
+ }
+ };
+ }
+
+ @Override
public void destroy() {
super.destroy();
