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

[Peter Levart]

On 12/05/2014 04:09 PM, Chris Hegarty wrote:
On 05/12/14 14:40, Peter Levart wrote:
On 12/05/2014 03:04 PM, Chris Hegarty wrote:

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].

What about the following:


private boolean closed;

public void close() throws IOException {
     try (OutputStream ostream = out) {
         if (!closed)  flush();
     }
     closed = true;
}

If flush throws, how is anyone supposed to know that they need to call 
close again, and that it was not the close that actually threw?

If they call close() again because they are used to pre 7015589 
behaviour, there is no harm if they call it as a consequence of flush() 
throwing and out.close() succeeding, since the underlying stream should 
have idempotent "successful" close(). That's the requirement of 
Closeable.close():

/**
* Closes this stream and releases any system resources associated
* with it. If the stream is already closed then invoking this
* method has no effect.
*
* <p> As noted in {@link AutoCloseable#close()}, cases where the
* close may fail require careful attention. It is strongly advised
* to relinquish the underlying resources and to internally
* <em>mark</em> the {@code Closeable} as closed, prior to throwing
* the {@code IOException}.
*
* @throws IOException if an I/O error occurs
*/

The specification does not say if the stream is to be considered 
"closed" when close() throws IOException. The advice does hint that 
though (and was added about 3 years ago).


close() should be a one-time shot.

I personally think that this was an oversight of the original change 
in 7015589, and not a deliberate behavior. My proposed change will 
restore the behavior of close to pre 7015589, though not swallow the 
IOException from flush.

The pre 7015589 behaviour did not suppress multiple close() calls to 
underlying stream.


-Chris.

I just think that a delegating class like FilterOutputStream should be 
as transparent as possible. If the underlying stream respects the 
contract and advice, the wrapped stream should too. If underlying stream 
needs some out-of-advice treatment, then wrapper like FilterOutputStream 
should not prevent that, because there are all kinds of streams out 
there. Or maybe it should - this is the only way to "fix" those streams ;-)

Just do it!

Regards, Peter


...this way the impact of the change is minimal and still addresses the
problem of JDK-8054565 
<https://bugs.openjdk.java.net/browse/JDK-8054565>.

If either flush() or out.close() throw exception the 1st time close() is
called, they will both be called also the 2nd time. Only after flush()
and out.close() return successfully, then further flush() invocations
are suppressed.

The alternative would be to not suppress flush() invocations, but just
any exception thrown from flush():


     private boolean closed;

     public void close() throws IOException {
         try (OutputStream ostream = out) {
             try {
                 flush();
             } catch (IOException | RuntimeException | Error e) {
                 if (!closed) throw e;
             }
         }
         closed = true;
     }


But I don't know if this is better or worse. It certainly could still be
squeezed under the spec which says:

      * The <code>close</code> method of <code>FilterOutputStream</code>
      * calls its <code>flush</code> method, and then calls the
      * <code>close</code> method of its underlying output stream.


Regards, Peter


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 <vita...@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().