[PATCH] JDK-8054565: FilterOutputStream.close may throw IOException if called twice and underlying flush or close fails
Chris Hegarty
chris.hegarty at oracle.com
Mon Dec 15 09:56:35 UTC 2014
On 15/12/14 05:21, Nathan Clement wrote:
> 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.
You are not violating anything, and it is quite reasonable to expect a
timely outcome in this situation.
I was asked, privately, to hold off pushing the change for this until
someone, who was involved in the original change that caused the
behavior change, had time to review. I will ping that individual again,
and move this forward.
-Chris.
> 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