Fix NPE when closing a stream from a different thread (#167)
* Fix NPE when closing a stream from a different thread
Than the one that opened the stream
* Add tests for closing streams in a different thread
diff --git a/commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/FileContentThreadData.java b/commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/FileContentThreadData.java
index 4e60f20..daf9283 100644
--- a/commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/FileContentThreadData.java
+++ b/commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/FileContentThreadData.java
@@ -79,7 +79,19 @@
}
void remove(final InputStream inputStream) {
- this.inputStreamList.remove(inputStream);
+ // this null-check (as well as the one in the other `remove` method) should not
+ // be needed because `remove` is called only in `DefaultFileContent.endInput` which
+ // should only be called after an input stream has been created and hence the `inputStreamList`
+ // variable initialized. However, `DefaultFileContent` uses this class per thread -
+ // so it is possible to get a stream, pass it to another thread and close it there -
+ // and that would lead to a NPE here if it weren't for that check. This "solution" here -
+ // adding a null-check - is really "bad" in the sense that it will fix a crash but there will
+ // be a leak because the input stream won't be removed from the original thread's `inputStreamList`.
+ // See https://github.com/apache/commons-vfs/pull/166 for more context.
+ // TODO: fix this problem
+ if (this.inputStreamList != null) {
+ this.inputStreamList.remove(inputStream);
+ }
}
Object removeRandomAccessContent(final int pos) {
@@ -87,7 +99,9 @@
}
void remove(final RandomAccessContent randomAccessContent) {
- this.randomAccessContentList.remove(randomAccessContent);
+ if (this.randomAccessContentList != null) {
+ this.randomAccessContentList.remove(randomAccessContent);
+ }
}
void setOutputStream(final DefaultFileContent.FileContentOutputStream outputStream) {
diff --git a/commons-vfs2/src/test/java/org/apache/commons/vfs2/provider/DefaultFileContentTest.java b/commons-vfs2/src/test/java/org/apache/commons/vfs2/provider/DefaultFileContentTest.java
index 405913f..14d646a 100644
--- a/commons-vfs2/src/test/java/org/apache/commons/vfs2/provider/DefaultFileContentTest.java
+++ b/commons-vfs2/src/test/java/org/apache/commons/vfs2/provider/DefaultFileContentTest.java
@@ -16,6 +16,7 @@
*/
package org.apache.commons.vfs2.provider;
+import java.io.Closeable;
import java.io.File;
import java.io.IOException;
import java.io.InputStream;
@@ -24,6 +25,7 @@
import org.apache.commons.lang3.ArrayUtils;
import org.apache.commons.lang3.StringUtils;
+import org.apache.commons.lang3.function.FailableFunction;
import org.apache.commons.vfs2.FileContent;
import org.apache.commons.vfs2.FileObject;
import org.apache.commons.vfs2.FileSystemManager;
@@ -80,6 +82,11 @@
}
@Test
+ public void testInputStreamClosedInADifferentThread() throws Exception {
+ testStreamClosedInADifferentThread(content -> content.getInputStream());
+ }
+
+ @Test
public void testMarkingWhenReadingEOS() throws Exception {
final File temp = File.createTempFile("temp-file-name", ".tmp");
final FileSystemManager fileSystemManager = VFS.getManager();
@@ -167,4 +174,28 @@
}
}
+ @Test
+ public void testOutputStreamClosedInADifferentThread() throws Exception {
+ testStreamClosedInADifferentThread(content -> content.getOutputStream());
+ }
+
+ private <T extends Closeable> void testStreamClosedInADifferentThread(FailableFunction<FileContent, T, IOException> getStream) throws Exception {
+ final File temp = File.createTempFile("temp-file-name", ".tmp");
+ final FileSystemManager fileSystemManager = VFS.getManager();
+
+ try (FileObject file = fileSystemManager.resolveFile(temp.getAbsolutePath())) {
+ T stream = getStream.apply(file.getContent());
+ boolean[] check = { false };
+ Thread thread = new Thread(() -> {
+ try {
+ stream.close();
+ } catch (IOException exception) {
+ }
+ check[0] = true;
+ });
+ thread.start();
+ thread.join();
+ Assert.assertTrue(check[0]);
+ }
+ }
}
