[PATCH] JDK-8054565: FilterOutputStream.close may throw IOException if called twice and underlying flush or close fails
Chris Hegarty
chris.hegarty at oracle.com
Fri Dec 5 22:37:04 UTC 2014
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