[PATCH] JDK-8054565: FilterOutputStream.close may throw IOException if called twice and underlying flush or close fails
Peter Levart
peter.levart at gmail.com
Fri Dec 5 14:40:27 UTC 2014
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;
}
...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