This is an automated email from the ASF dual-hosted git repository.
ggregory pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/commons-vfs.git
The following commit(s) were added to refs/heads/master by this push:
new 8093086 Fix NPE when closing a stream from a different thread (#167)
8093086 is described below
commit 809308694ac1b00221aa3726e66beeb0ac2c7ae1
Author: Boris Petrov <[email protected]>
AuthorDate: Mon Mar 15 14:51:31 2021 +0200
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
---
.../vfs2/provider/FileContentThreadData.java | 18 +++++++++++--
.../vfs2/provider/DefaultFileContentTest.java | 31 ++++++++++++++++++++++
2 files changed, 47 insertions(+), 2 deletions(-)
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 @@ class FileContentThreadData {
}
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 @@ class FileContentThreadData {
}
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 java.nio.charset.StandardCharsets;
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 @@ public class DefaultFileContentTest {
}
@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 @@ public class DefaultFileContentTest {
}
}
+ @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]);
+ }
+ }
}
