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>tags/v4.11.0.201803080745-r
@@ -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; | |||
} |
@@ -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; | |||
} | |||
} |
@@ -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 |
@@ -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; | |||
} |
@@ -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(); | |||
} | |||
@@ -313,6 +313,26 @@ public abstract class TemporaryBuffer extends OutputStream { | |||
return new BlockInputStream(); | |||
} | |||
/** | |||
* 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. | |||
*/ | |||
@@ -505,6 +525,20 @@ public abstract class TemporaryBuffer extends OutputStream { | |||
return new FileInputStream(onDiskFile); | |||
} | |||
@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(); |