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