[PATCH] JDK-8054565: FilterOutputStream.close may throw IOException if called twice and underlying flush or close fails
Nathan Clement
nathan.a.clement at hotmail.com
Mon Dec 15 05:21:35 UTC 2014
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.clement at hotmail.com
> To: chris.hegarty at oracle.com; peter.levart at 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 at 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.hegarty at oracle.com
> Date: Fri, 5 Dec 2014 22:37:04 +0000
> CC: pavel.rappo at oracle.com; nathan.a.clement at hotmail.com; core-libs-dev at openjdk.java.net
> To: peter.levart at gmail.com
>
>
> On 5 Dec 2014, at 17:44, Peter Levart <peter.levart at 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 <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().
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
More information about the core-libs-dev
mailing list