Revert *InputStream.read() should return -1 (EOF) after the stream is

closed - The InputStream#read(*) Javadocs suggests we should throw an IOException in that case - BOMInputStream - ProxyInputStream - BufferedFileChannelInputStream - MemoryMappedFileInputStream - RandomAccessFileInputStream
apache · Aug 7, 2024 · cc62ff7 · cc62ff7
1 parent 075b3f6
commit cc62ff7
Show file tree

Hide file tree

Showing 17 changed files with 143 additions and 83 deletions.
diff --git a/src/changes/changes.xml b/src/changes/changes.xml
@@ -74,20 +74,14 @@ The <action> type attribute can be add,update,fix,remove.
       <action dev="ggregory" type="fix"                due-to="Gary Gregory">CircularInputStream.available() should return 0 when the stream is closed.</action>
       <action dev="ggregory" type="fix"                due-to="Gary Gregory">InfiniteCircularInputStream.available() should return 0 when the stream is closed.</action>
       <action dev="ggregory" type="fix"                due-to="Gary Gregory">ChecksumInputStream(InputStream, Checksum, long, long) should fail-fast on null Checksum input.</action>
-      <action dev="ggregory" type="fix"                due-to="Gary Gregory">BufferedFileChannelInputStream.read() should return -1 (EOF) after the stream is closed.</action>
-      <action dev="ggregory" type="fix"                due-to="Gary Gregory">BOMInputStream.read() should return -1 (EOF) after the stream is closed.</action>
       <action dev="ggregory" type="fix"                due-to="Gary Gregory">Deprecate NullInputStream.INSTANCE in favor of constructors.</action>
       <action dev="ggregory" type="fix"                due-to="Gary Gregory">NullInputStream.available() should return 0 after the stream is closed.</action>
-      <action dev="ggregory" type="fix"                due-to="Gary Gregory">NullInputStream.read() should return -1 (EOF) after the stream is closed.</action>
       <action dev="ggregory" type="fix"                due-to="Gary Gregory">MemoryMappedFileInputStream.available() should return 0 after the stream is closed.</action>
-      <action dev="ggregory" type="fix"                due-to="Gary Gregory">MemoryMappedFileInputStream.read() should return -1 (EOF) after the stream is closed.</action>
-      <action dev="ggregory" type="fix"                due-to="Gary Gregory">ProxyInputStream.read() should return -1 (EOF) after the stream is closed.</action>
       <action dev="ggregory" type="fix"                due-to="Gary Gregory">RandomAccessFileInputStream.available() should return 0 after the stream is closed.</action>
-      <action dev="ggregory" type="fix"                due-to="Gary Gregory">RandomAccessFileInputStream.read() should return -1 (EOF) after the stream is closed.</action>
       <action dev="ggregory" type="fix"                due-to="Gary Gregory">ReaderInputStream.available() should return 0 after the stream is closed.</action>
-      <action dev="ggregory" type="fix"                due-to="Gary Gregory">ReaderInputStream.read() should return -1 (EOF) after the stream is closed.</action>
       <action dev="ggregory" type="fix"                due-to="Gary Gregory">AutoCloseInputStream does not call handleIOException() on close() when the proxied stream throws an IOException.</action>
       <action dev="ggregory" type="fix"                due-to="Gary Gregory">BoundedInputStream does not call handleIOException() on close() when the proxied stream throws an IOException.</action>
+      <action dev="ggregory" type="fix"                due-to="Gary Gregory">NullInputStream.read() can be configured to return -1 (EOF) after the stream is closed.</action>
       <!-- UPDATE -->
       <action dev="ggregory" type="update"             due-to="Dependabot">Bump tests commons.bytebuddy.version from 1.14.13 to 1.14.18 #615, #621, #631, #635, #642.</action>
       <action dev="ggregory" type="update"             due-to="Dependabot">Bump tests commons-codec:commons-codec from 1.16.1 to 1.17.1 #644.</action>

diff --git a/src/main/java/org/apache/commons/io/input/AbstractInputStream.java b/src/main/java/org/apache/commons/io/input/AbstractInputStream.java
@@ -27,11 +27,32 @@
  */
 public abstract class AbstractInputStream extends InputStream {
 
+    /**
+     * Throws an IOException on false input.
+     *
+     * @param isOpen whether the stream is open or not.
+     * @throws IOException if this instance is closed.
+     */
+    static void checkOpen(final boolean isOpen) throws IOException {
+        if (!isOpen) {
+            throw new IOException("The stream is closed.");
+        }
+    }
+
     /**
      * Whether {@link #close()} completed successfully.
      */
     private boolean closed;
 
+    /**
+     * Checks if this instance is closed and throws an IOException if so.
+     *
+     * @throws IOException if this instance is closed.
+     */
+    void checkOpen() throws IOException {
+        checkOpen(!isClosed());
+    }
+
     @Override
     public void close() throws IOException {
         super.close();

diff --git a/src/main/java/org/apache/commons/io/input/BOMInputStream.java b/src/main/java/org/apache/commons/io/input/BOMInputStream.java
@@ -419,9 +419,7 @@ private boolean matches(final ByteOrderMark bom) {
      */
     @Override
     public int read() throws IOException {
-        if (isClosed()) {
-            return EOF;
-        }
+        checkOpen();
         final int b = readFirstBytes();
         return b >= 0 ? b : in.read();
     }

diff --git a/src/main/java/org/apache/commons/io/input/BufferedFileChannelInputStream.java b/src/main/java/org/apache/commons/io/input/BufferedFileChannelInputStream.java
@@ -243,9 +243,7 @@ public synchronized int read(final byte[] b, final int offset, int len) throws I
      * @throws IOException if an I/O error occurs.
      */
     private boolean refill() throws IOException {
-        if (!fileChannel.isOpen()) {
-            return false;
-        }
+        AbstractInputStream.checkOpen(fileChannel.isOpen());
         if (!byteBuffer.hasRemaining()) {
             byteBuffer.clear();
             int nRead = 0;

diff --git a/src/main/java/org/apache/commons/io/input/MemoryMappedFileInputStream.java b/src/main/java/org/apache/commons/io/input/MemoryMappedFileInputStream.java
@@ -170,12 +170,6 @@ public int available() throws IOException {
         return buffer.remaining();
     }
 
-    private void checkOpen() throws IOException {
-        if (isClosed()) {
-            throw new IOException("Stream closed");
-        }
-    }
-
     private void cleanBuffer() {
         if (ByteBufferCleaner.isSupported() && buffer.isDirect()) {
             ByteBufferCleaner.clean(buffer);
@@ -210,9 +204,7 @@ private void nextBuffer() throws IOException {
 
     @Override
     public int read() throws IOException {
-        if (isClosed()) {
-            return EOF;
-        }
+        checkOpen();
         if (!buffer.hasRemaining()) {
             nextBuffer();
             if (!buffer.hasRemaining()) {

diff --git a/src/main/java/org/apache/commons/io/input/ProxyInputStream.java b/src/main/java/org/apache/commons/io/input/ProxyInputStream.java
@@ -143,6 +143,15 @@ protected void beforeRead(final int n) throws IOException {
         // no-op default
     }
 
+    /**
+     * Checks if this instance is closed and throws an IOException if so.
+     *
+     * @throws IOException if this instance is closed.
+     */
+    void checkOpen() throws IOException {
+        AbstractInputStream.checkOpen(!isClosed());
+    }
+
     /**
      * Invokes the delegate's {@link InputStream#close()} method.
      *
@@ -210,9 +219,6 @@ public boolean markSupported() {
      */
     @Override
     public int read() throws IOException {
-        if (isClosed()) {
-            return EOF;
-        }
         try {
             beforeRead(1);
             final int b = in.read();
@@ -328,4 +334,5 @@ public long skip(final long n) throws IOException {
     public InputStream unwrap() {
         return in;
     }
+
 }
diff --git a/src/main/java/org/apache/commons/io/input/RandomAccessFileInputStream.java b/src/main/java/org/apache/commons/io/input/RandomAccessFileInputStream.java
@@ -22,7 +22,6 @@
 import java.io.RandomAccessFile;
 import java.util.Objects;
 
-import org.apache.commons.io.IOUtils;
 import org.apache.commons.io.RandomAccessFileMode;
 import org.apache.commons.io.build.AbstractOrigin;
 import org.apache.commons.io.build.AbstractStreamBuilder;
@@ -210,7 +209,7 @@ public boolean isCloseOnClose() {
 
     @Override
     public int read() throws IOException {
-        return isClosed() ? IOUtils.EOF : randomAccessFile.read();
+        return randomAccessFile.read();
     }
 
     @Override

diff --git a/src/main/java/org/apache/commons/io/input/ReaderInputStream.java b/src/main/java/org/apache/commons/io/input/ReaderInputStream.java
@@ -408,9 +408,7 @@ CharsetEncoder getCharsetEncoder() {
      */
     @Override
     public int read() throws IOException {
-        if (isClosed()) {
-            return EOF;
-        }
+        checkOpen();
         for (;;) {
             if (encoderOut.hasRemaining()) {
                 return encoderOut.get() & 0xFF;

diff --git a/src/test/java/org/apache/commons/io/input/BOMInputStreamTest.java b/src/test/java/org/apache/commons/io/input/BOMInputStreamTest.java
@@ -37,7 +37,6 @@
 import javax.xml.parsers.ParserConfigurationException;
 
 import org.apache.commons.io.ByteOrderMark;
-import org.apache.commons.io.IOUtils;
 import org.apache.commons.lang3.SystemProperties;
 import org.junit.jupiter.api.Test;
 import org.w3c.dom.Document;
@@ -429,12 +428,11 @@ public void testNoBoms() throws Exception {
     @Test
     public void testReadAfterClose() throws Exception {
         final byte[] data = { 'A', 'B', 'C', 'D' };
-        final InputStream shadow;
         try (InputStream in = BOMInputStream.builder().setInputStream(createUtf8Input(data, true)).get()) {
             assertEquals(7, in.available());
-            shadow = in;
+            in.close();
+            assertThrows(IOException.class, in::read);
         }
-        assertEquals(IOUtils.EOF, shadow.read());
     }
 
     @Test

diff --git a/src/test/java/org/apache/commons/io/input/BufferedFileChannelInputStreamTest.java b/src/test/java/org/apache/commons/io/input/BufferedFileChannelInputStreamTest.java
@@ -16,15 +16,13 @@
  */
 package org.apache.commons.io.input;
 
-import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
 import java.io.IOException;
 import java.io.InputStream;
 import java.nio.file.StandardOpenOption;
 
-import org.apache.commons.io.IOUtils;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
@@ -71,7 +69,7 @@ public void testBuilderGet() {
     public void testReadAfterClose() throws Exception {
         for (final InputStream inputStream : inputStreams) {
             inputStream.close();
-            assertEquals(IOUtils.EOF, inputStream.read());
+            assertThrows(IOException.class, inputStream::read);
         }
     }
 

diff --git a/src/test/java/org/apache/commons/io/input/MemoryMappedFileInputStreamTest.java b/src/test/java/org/apache/commons/io/input/MemoryMappedFileInputStreamTest.java
@@ -19,6 +19,7 @@
 import static org.apache.commons.lang3.ArrayUtils.EMPTY_BYTE_ARRAY;
 import static org.junit.jupiter.api.Assertions.assertArrayEquals;
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 
 import java.io.IOException;
 import java.io.InputStream;
@@ -146,16 +147,14 @@ public void testReadAfterClose() throws IOException {
         }
     }
 
-    @SuppressWarnings("resource")
     @ParameterizedTest
     @MethodSource(AbstractInputStreamTest.ARRAY_LENGTHS_NAME)
     public void testReadAfterClose(final int len) throws Exception {
         final Path file = createTestFile(len);
-        final InputStream shadow;
         try (InputStream inputStream = newInputStream(file, 1024)) {
-            shadow = inputStream;
+            inputStream.close();
+            assertThrows(IOException.class, inputStream::read);
         }
-        assertEquals(IOUtils.EOF, shadow.read());
     }
 
     @Test

diff --git a/src/test/java/org/apache/commons/io/input/MessageDigestCalculatingInputStreamTest.java b/src/test/java/org/apache/commons/io/input/MessageDigestCalculatingInputStreamTest.java
@@ -20,10 +20,13 @@
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNotEquals;
 import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 
 import java.io.ByteArrayInputStream;
 import java.io.IOException;
 import java.io.InputStream;
+import java.nio.file.Files;
+import java.nio.file.Paths;
 import java.security.MessageDigest;
 
 import org.apache.commons.codec.digest.DigestUtils;
@@ -39,8 +42,12 @@
 public class MessageDigestCalculatingInputStreamTest {
 
     private InputStream createInputStream() throws IOException {
-        return MessageDigestCalculatingInputStream.builder()
-                .setInputStream(new ByteArrayInputStream(MessageDigestInputStreamTest.generateRandomByteStream(256))).get();
+        final ByteArrayInputStream origin = new ByteArrayInputStream(MessageDigestInputStreamTest.generateRandomByteStream(256));
+        return createInputStream(origin);
+    }
+
+    private MessageDigestCalculatingInputStream createInputStream(final InputStream origin) throws IOException {
+        return MessageDigestCalculatingInputStream.builder().setInputStream(origin).get();
     }
 
     @SuppressWarnings("resource")
@@ -111,15 +118,23 @@ public void testNormalUse() throws Exception {
         }
     }
 
-    @SuppressWarnings("resource")
     @Test
-    public void testReadAfterClose() throws Exception {
-        final InputStream shadow;
+    public void testReadAfterClose_ByteArrayInputStream() throws Exception {
         try (InputStream in = createInputStream()) {
-            assertTrue(in.available() > 0);
-            shadow = in;
+            in.close();
+            // ByteArrayInputStream does not throw on a closed stream.
+            assertNotEquals(IOUtils.EOF, in.read());
+        }
+    }
+
+    @SuppressWarnings("resource")
+    @Test
+    public void testReadAfterClose_ChannelInputStream() throws Exception {
+        try (InputStream in = createInputStream(Files.newInputStream(Paths.get("src/test/resources/org/apache/commons/io/abitmorethan16k.txt")))) {
+            in.close();
+            // ChannelInputStream throws when closed
+            assertThrows(IOException.class, in::read);
         }
-        assertEquals(IOUtils.EOF, shadow.read());
     }
 
 }
diff --git a/src/test/java/org/apache/commons/io/input/MessageDigestInputStreamTest.java b/src/test/java/org/apache/commons/io/input/MessageDigestInputStreamTest.java
@@ -25,6 +25,8 @@
 import java.io.ByteArrayInputStream;
 import java.io.IOException;
 import java.io.InputStream;
+import java.nio.file.Files;
+import java.nio.file.Paths;
 import java.security.MessageDigest;
 import java.security.NoSuchAlgorithmException;
 import java.util.Random;
@@ -47,10 +49,15 @@ static byte[] generateRandomByteStream(final int pSize) {
     }
 
     private InputStream createInputStream() throws IOException, NoSuchAlgorithmException {
+        return createInputStream(new ByteArrayInputStream(MessageDigestInputStreamTest.generateRandomByteStream(256)));
+    }
+
+    private InputStream createInputStream(final InputStream origin) throws IOException, NoSuchAlgorithmException {
         // @formatter:off
         return MessageDigestInputStream.builder()
                 .setMessageDigest(MessageDigest.getInstance(MessageDigestAlgorithms.SHA_512))
-                .setInputStream(new ByteArrayInputStream(MessageDigestInputStreamTest.generateRandomByteStream(256))).get();
+                .setInputStream(origin)
+                .get();
         // @formatter:on
     }
 
@@ -98,15 +105,22 @@ public void testNormalUse() throws Exception {
         }
     }
 
-    @SuppressWarnings("resource")
     @Test
-    public void testReadAfterClose() throws Exception {
-        final InputStream shadow;
+    public void testReadAfterClose_ByteArrayInputStream() throws Exception {
         try (InputStream in = createInputStream()) {
-            assertTrue(in.available() > 0);
-            shadow = in;
+            in.close();
+            assertNotEquals(IOUtils.EOF, in.read());
+        }
+    }
+
+    @SuppressWarnings("resource")
+    @Test
+    public void testReadAfterClose_ChannelInputStream() throws Exception {
+        try (InputStream in = createInputStream(Files.newInputStream(Paths.get("src/test/resources/org/apache/commons/io/abitmorethan16k.txt")))) {
+            in.close();
+            // ChannelInputStream throws when closed
+            assertThrows(IOException.class, in::read);
         }
-        assertEquals(IOUtils.EOF, shadow.read());
     }
 
 }