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>

6 files changed, 88 insertions, 33 deletions

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 fccbae7955..217e46f48e 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 class CleanFilter extends FilterCommand {
 				}
 				LfsPointer lfsPointer = new LfsPointer(loid, size);
 				lfsPointer.encode(out);
+				in.close();
 				out.close();
 				return -1;
 			}
@@ -174,6 +175,7 @@ public class CleanFilter extends FilterCommand {
 			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 142e74df6a..5c0d3b47a7 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 @@ public class SmudgeFilter extends FilterCommand {
 	 * @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 class SmudgeFilter extends FilterCommand {
 	 *            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 class SmudgeFilter extends FilterCommand {
 	/** {@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 aae00764f6..c4357d1297 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 abstract class FilterCommand {
 	 * 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 4ebf01febc..68cc7cb580 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 @@ public abstract class WorkingTreeIterator extends AbstractTreeIterator {
 				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 @@ public abstract class WorkingTreeIterator extends AbstractTreeIterator {
 						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 10fe5642a5..9200916495 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 class LfsFactory {
 		 *             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 31abb7c173..dd933a0294 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 abstract class TemporaryBuffer extends OutputStream {
 	}
 
 	/**
+	 * 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 abstract class TemporaryBuffer extends OutputStream {
 		}
 
 		@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();