Hi,

What's the process for finalizing a decision and getting this bug fixed?

Apologies if I'm violating a social norm by asking this - I'm new to this 
process and I'm not sure exactly how decisions are made.

Thanks,

Nathan

> From: nathan.a.clem...@hotmail.com
> To: chris.hega...@oracle.com; peter.lev...@gmail.com
> Subject: RE: [PATCH] JDK-8054565: FilterOutputStream.close may throw  
> IOException if called twice and underlying flush or close fails
> Date: Tue, 9 Dec 2014 16:17:29 +1100
> CC: core-libs-dev@openjdk.java.net
> 
> Hi,
> 
> I'd be happy with any of the solutions discussed here.  In our case, we're 
> only calling close() multiple times as a result of the way that 
> try-with-resources works with wrapped streams.
> 
> Thanks,
> 
> Nathan
> 
> Subject: Re: [PATCH] JDK-8054565: FilterOutputStream.close may throw 
> IOException if called twice and underlying flush or close fails
> From: chris.hega...@oracle.com
> Date: Fri, 5 Dec 2014 22:37:04 +0000
> CC: pavel.ra...@oracle.com; nathan.a.clem...@hotmail.com; 
> core-libs-dev@openjdk.java.net
> To: peter.lev...@gmail.com
> 
> 
> On 5 Dec 2014, at 17:44, Peter Levart <peter.lev...@gmail.com> wrote:
>   
>     
>   
>   
>     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,
> I don’t think I have every seen any code that catches the IOException from 
> close() , and tries to close again. But maybe someone does this?
> From AutoCloseable:  “Cases where the close operation may fail require 
> careful attention by implementers. It is strongly advised to relinquish the 
> underlying resources and to internally mark the resource as closed, prior to 
> throwing the exception. The close method is unlikely to be invoked more than 
> once and so this ensures that the resources are released in a timely manner. 
> Furthermore it reduces problems that could arise when the resource wraps, or 
> is wrapped, by another resource.”
> I think the changes I am proposing follows this advise ( as best we can ).
>  there is no harm if they call it as a consequence of
>     flush() throwing and out.close() succeeding, since the underlying
>     stream should 
> Yes, “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.
> 
> FilterOutputStream is Closeable, but what you are suggesting is that 
> subsequent calls to close would have an effect, they may call flush.
> 
>      * <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).
> Re-reading the advise in AutoCloseable and Closeable, I do not see any 
> problem with the changes I suggested.
> -Chris.
> 
>       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().
>               
> 
>             
>             
> 
>           
>         
>         
> 
>       
>     
>     
> 
>   
> 
> 
>                                         
                                          

Reply via email to