[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
Tue Dec 9 05:17:29 UTC 2014
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