[PATCH] JDK-8054565: FilterOutputStream.close may throw IOException if called twice and underlying flush or close fails

[Chris Hegarty]

On 05/12/14 11:38, Pavel Rappo wrote:
> +1, I couldn’t say better

Right. This bug should only try to address the change in behavior that 
was inadvertently introduced by 7015589[1][2].

I don't see any reason to make 'closed' protected ( part of the public 
Java SE API ), something like:

diff --git a/src/java.base/share/classes/java/io/FilterOutputStream.java 
b/src/java.base/share/classes/java/io/FilterOutputStream.java
--- a/src/java.base/share/classes/java/io/FilterOutputStream.java
+++ b/src/java.base/share/classes/java/io/FilterOutputStream.java
@@ -48,6 +48,8 @@
       */
      protected OutputStream out;

+    private boolean closed; // false
+
      /**
       * Creates an output stream filter built on top of the specified
       * underlying output stream.
@@ -154,6 +156,9 @@
       */
      @SuppressWarnings("try")
      public void close() throws IOException {
+        if (closed)
+            return;
+        closed = true;
          try (OutputStream ostream = out) {
              flush();
          }

diff --git a/test/java/io/etc/FailingFlushAndClose.java 
b/test/java/io/etc/FailingFlushAndClose.java
--- a/test/java/io/etc/FailingFlushAndClose.java
+++ b/test/java/io/etc/FailingFlushAndClose.java
@@ -25,7 +25,7 @@

  /**
   * @test
- * @bug 7015589
+ * @bug 7015589 8054565
   * @summary Test that buffering streams are considered closed even 
when the
   *    close or flush from the underlying stream fails.
   */
@@ -165,7 +165,7 @@
          }
      }

-    static void testFailingClose(InputStream in) throws IOException {
+    static InputStream testFailingClose(InputStream in) throws 
IOException {
          System.out.println(in.getClass());
          in.read(new byte[100]);
          try {
@@ -176,9 +176,10 @@
              in.read(new byte[100]);
              fail("read did not fail");
          } catch (IOException expected) { }
+        return in;
      }

-    static void testFailingClose(OutputStream out) throws IOException {
+    static OutputStream testFailingClose(OutputStream out) throws 
IOException {
          System.out.println(out.getClass());
          out.write(1);
          try {
@@ -190,9 +191,10 @@
              if (!(out instanceof BufferedOutputStream))
                  fail("write did not fail");
          } catch (IOException expected) { }
+        return out;
      }

-    static void testFailingFlush(OutputStream out) throws IOException {
+    static OutputStream testFailingFlush(OutputStream out) throws 
IOException {
          System.out.println(out.getClass());
          out.write(1);
          try {
@@ -206,9 +208,27 @@
                  fail("close did not fail");
              } catch (IOException expected) { }
          }
+        return out;
      }

-    static void testFailingClose(Reader r) throws IOException {
+    static void closeAgain(InputStream in) throws IOException {
+        // assert the given stream should already be closed.
+        try {
+            in.close();
+        } catch (IOException expected) {
+            fail("unexpected IOException from subsequent close");
+        }
+    }
+    static void closeAgain(OutputStream out) throws IOException {
+        // assert the given stream should already be closed.
+        try {
+            out.close();
+        } catch (IOException expected) {
+            fail("unexpected IOException from subsequent close");
+        }
+    }
+
+    static Reader testFailingClose(Reader r) throws IOException {
          System.out.println(r.getClass());
          r.read(new char[100]);
          try {
@@ -219,9 +239,10 @@
              r.read(new char[100]);
              fail("read did not fail");
          } catch (IOException expected) { }
+        return r;
      }

-    static void testFailingClose(Writer w) throws IOException {
+    static Writer testFailingClose(Writer w) throws IOException {
          System.out.println(w.getClass());
          w.write("message");
          try {
@@ -232,9 +253,10 @@
              w.write("another message");
              fail("write did not fail");
          } catch (IOException expected) { }
+        return w;
      }

-    static void testFailingFlush(Writer w) throws IOException {
+    static Writer testFailingFlush(Writer w) throws IOException {
          System.out.println(w.getClass());
          w.write("message");
          try {
@@ -249,18 +271,38 @@
                  fail("close did not fail");
              } catch (IOException expected) { }
          }
+        return w;
+    }
+
+    static Reader closeAgain(Reader r) throws IOException {
+        // assert the given stream should already be closed.
+        try {
+            r.close();
+        } catch (IOException expected) {
+            fail("unexpected IOException from subsequent close");
+        }
+        return r;
+    }
+    static Writer closeAgain(Writer w) throws IOException {
+        // assert the given stream should already be closed.
+        try {
+            w.close();
+        } catch (IOException expected) {
+            fail("unexpected IOException from subsequent close");
+        }
+        return w;
      }

      public static void main(String[] args) throws IOException {

-        testFailingClose(new BufferedInputStream(new 
FailingCloseInputStream()));
-        testFailingClose(new BufferedOutputStream(new 
FailingCloseOutputStream()));
+        closeAgain(testFailingClose(new BufferedInputStream(new 
FailingCloseInputStream())));
+        closeAgain(testFailingClose(new BufferedOutputStream(new 
FailingCloseOutputStream())));

-        testFailingClose(new BufferedReader(new FailingCloseReader()));
-        testFailingClose(new BufferedWriter(new FailingCloseWriter()));
+        closeAgain(testFailingClose(new BufferedReader(new 
FailingCloseReader())));
+        closeAgain(testFailingClose(new BufferedWriter(new 
FailingCloseWriter())));

-        testFailingFlush(new BufferedOutputStream(new 
FailingFlushOutputStream()));
-        testFailingFlush(new BufferedWriter(new FailingFlushWriter()));
+        closeAgain(testFailingFlush(new BufferedOutputStream(new 
FailingFlushOutputStream())));
+        closeAgain(testFailingFlush(new BufferedWriter(new 
FailingFlushWriter())));

          if (failed > 0)
              throw new RuntimeException(failed + " test(s) failed - see 
log for details");


-Chris.

[1] https://bugs.openjdk.java.net/browse/JDK-7015589
[2] http://hg.openjdk.java.net/jdk8/jdk8/jdk/rev/759aa847dcaf


> -Pavel
>
>> On 5 Dec 2014, at 01:18, Vitaly Davidovich <vitalyd at gmail.com> wrote:
>>
>> Attempting to make close() atomic just makes the next reader
>> confused when the rest of the class isn't and may also penalize single
>> threaded callers of close().
>