[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 15:09:10 UTC 2014
Or, maybe a 3rd, more refined variant:
private boolean closed;
public void close() throws IOException {
Exception exc = null;
try {
flush();
} catch (IOException | RuntimeException e) {
if (!closed) exc = e;
}
try {
out.close();
closed = true;
} catch (IOException | RuntimeException e) {
if (exc == null) {
exc = e;
} else {
exc.addSuppressed(e);
}
}
if (exc != null) {
if (exc instanceof IOException) {
throw (IOException) exc;
} else {
throw (RuntimeException) exc;
}
}
}
The difference being that underlying stream is considered closed (and
consequently exceptions from further flushes ignored) as soon as
out.close() returns successfully.
Regards, Peter
On 12/05/2014 03:40 PM, 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;
> }
>
>
> ...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