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