FilterOutputStream.close() throws exception from flush()

[Bernd Eckenfels-4]

Hello,

back in 2011 there was a discussion about the new changed behavior of
FilterOutputStream (and BufferedOutputStream) in regards to not anymore
swalloging IOExceptions from flush() on this list (thats where I got
the subject from).

This was generally a very good improvement (and I am glad that
https://bugs.openjdk.java.net/browse/JDK-6335274 thereby got fixed).
However the implementation with the try-with-resource has a problem:
when flush() and close() report the same exception instance the
construction of the suppressed exception will actually fail with an
IllegalArgumentException.

This IllegalArgumentException of Throwable.addSuppressed is very
unfortunate (it would be better simply ignore it).

Anyway, this new behavior broke a Unit-Test for Apache VFS as you can
see here: https://issues.apache.org/jira/browse/VFS-521

I think this can only be fixed in Throwable by avoiding this
IllegalArgumentException or by the close() method not using
try-with-resource.

For reference, according to this changeset other locations are affected
as well:
http://hg.openjdk.java.net/jdk8/jdk8/jdk/rev/759aa847dcaf

Gruss
Bernd

PS: I wrote a german blogpost about the FilterOutputStream here:
http://itblog.eckenfels.net/archives/505-FOS-considered-harmfull.html

The following exception is produced by the testcode:

Exception in thread "main" java.lang.IllegalArgumentException: Self-suppression not permitted
        at java.lang.Throwable.addSuppressed(Throwable.java:1043)
        at java.io.FilterOutputStream.close(FilterOutputStream.java:159)
        at testfos.Main.main(Main.java:6)
Caused by: java.io.IOException: remebered IO Exception
        at testfos.FailingOS.<init>(FailingOS.java:10)
        at testfos.Main.main(Main.java:5)

package testfos;

public class Main {
    public static void main(String[] args) throws java.io.IOException {
        java.io.BufferedOutputStream buf = new java.io.BufferedOutputStream(new testfos.FailingOS());
        buf.close(); // expected: IOException
    }
}

package testfos;

public class FailingOS
    extends java.io.OutputStream
{
    private java.io.IOException ex;

    public FailingOS()
    {
        ex = new java.io.IOException("remebered IO Exception");
    }

    public void write(int b) throws java.io.IOException { }

    public void flush() throws java.io.IOException
    { throw ex; }

    public void close() throws java.io.IOException
    { throw ex; }
}

[Alan Bateman]

On 03/05/2014 01:04, Bernd Eckenfels wrote:

> Hello,
>
> back in 2011 there was a discussion about the new changed behavior of
> FilterOutputStream (and BufferedOutputStream) in regards to not anymore
> swalloging IOExceptions from flush() on this list (thats where I got
> the subject from).
>
> This was generally a very good improvement (and I am glad that
> https://bugs.openjdk.java.net/browse/JDK-6335274 thereby got fixed).
> However the implementation with the try-with-resource has a problem:
> when flush() and close() report the same exception instance the
> construction of the suppressed exception will actually fail with an
> IllegalArgumentException.
>
> This IllegalArgumentException of Throwable.addSuppressed is very
> unfortunate (it would be better simply ignore it).
>
> Anyway, this new behavior broke a Unit-Test for Apache VFS as you can
> see here: https://issues.apache.org/jira/browse/VFS-521
>
> I think this can only be fixed in Throwable by avoiding this
> IllegalArgumentException or by the close() method not using
> try-with-resource.
>
> For reference, according to this changeset other locations are affected
> as well:
> http://hg.openjdk.java.net/jdk8/jdk8/jdk/rev/759aa847dcaf
>

Thanks this bug report, I've created JDK-8042377 to track it. I don't
think this has come up before, probably because flush and close are more
likely to throw new exceptions rather than equal (cached) exceptions.

I don't see this issue as driver to changing Throwable to support self
suppression but we can change BufferedWriter and FilteredOuputStream to
only add the suppressed exception when they are not equal.

-Alan.

[Bernd Eckenfels-4]

Thanks Alan,

I just want to add that I think the IAE is quite in a unfortunate location
when it is caused by TWR. Especially for that usecase silently ignoring the
add is more robust.

If you look at Stackoverflow or Google there ard already other People
confused by it, it happens all over the place.

https://www.google.de/search?q=close+IllegalArgumentException+self-suppression

Bernd
Am 05.05.2014 11:32 schrieb "Alan Bateman" <[hidden email]>:

> On 03/05/2014 01:04, Bernd Eckenfels wrote:
>
>> Hello,
>>
>> back in 2011 there was a discussion about the new changed behavior of
>> FilterOutputStream (and BufferedOutputStream) in regards to not anymore
>> swalloging IOExceptions from flush() on this list (thats where I got
>> the subject from).
>>
>> This was generally a very good improvement (and I am glad that
>> https://bugs.openjdk.java.net/browse/JDK-6335274 thereby got fixed).
>> However the implementation with the try-with-resource has a problem:
>> when flush() and close() report the same exception instance the
>> construction of the suppressed exception will actually fail with an
>> IllegalArgumentException.
>>
>> This IllegalArgumentException of Throwable.addSuppressed is very
>> unfortunate (it would be better simply ignore it).
>>
>> Anyway, this new behavior broke a Unit-Test for Apache VFS as you can
>> see here: https://issues.apache.org/jira/browse/VFS-521
>>
>> I think this can only be fixed in Throwable by avoiding this
>> IllegalArgumentException or by the close() method not using
>> try-with-resource.
>>
>> For reference, according to this changeset other locations are affected
>> as well:
>> http://hg.openjdk.java.net/jdk8/jdk8/jdk/rev/759aa847dcaf
>>
>>  Thanks this bug report, I've created JDK-8042377 to track it. I don't
> think this has come up before, probably because flush and close are more
> likely to throw new exceptions rather than equal (cached) exceptions.
>
> I don't see this issue as driver to changing Throwable to support self
> suppression but we can change BufferedWriter and FilteredOuputStream to
> only add the suppressed exception when they are not equal.
>
> -Alan.
>

[Peter Levart]

On 05/05/2014 11:31 AM, Alan Bateman wrote:

> On 03/05/2014 01:04, Bernd Eckenfels wrote:
>> Hello,
>>
>> back in 2011 there was a discussion about the new changed behavior of
>> FilterOutputStream (and BufferedOutputStream) in regards to not anymore
>> swalloging IOExceptions from flush() on this list (thats where I got
>> the subject from).
>>
>> This was generally a very good improvement (and I am glad that
>> https://bugs.openjdk.java.net/browse/JDK-6335274 thereby got fixed).
>> However the implementation with the try-with-resource has a problem:
>> when flush() and close() report the same exception instance the
>> construction of the suppressed exception will actually fail with an
>> IllegalArgumentException.
>>
>> This IllegalArgumentException of Throwable.addSuppressed is very
>> unfortunate (it would be better simply ignore it).
>>
>> Anyway, this new behavior broke a Unit-Test for Apache VFS as you can
>> see here: https://issues.apache.org/jira/browse/VFS-521
>>
>> I think this can only be fixed in Throwable by avoiding this
>> IllegalArgumentException or by the close() method not using
>> try-with-resource.
>>
>> For reference, according to this changeset other locations are affected
>> as well:
>> http://hg.openjdk.java.net/jdk8/jdk8/jdk/rev/759aa847dcaf
>>
> Thanks this bug report, I've created JDK-8042377 to track it. I don't
> think this has come up before, probably because flush and close are
> more likely to throw new exceptions rather than equal (cached)
> exceptions.

Hi Alan,

There has been a discussion about a year ago on this list:

http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-April/015947.html


>
> I don't see this issue as driver to changing Throwable to support self
> suppression but we can change BufferedWriter and FilteredOuputStream
> to only add the suppressed exception when they are not equal.

It's try-with-resources construct that adds suppressed exceptions to
other exceptions, OuputStreams are just throwing them. And
(Buffered)FilteredOuputStream doesn't do any exception caching - it just
propagates exceptions thrown by underlying OuputStream. If underlying
OuputStream is throwing the same instance on flush() and on close() than
we have problem. The only thing a FilteredOuputStream could do was to
actually cache the exception thrown by underlying stream's flush()
method and compare it by reference with the exception thrown by
underlying stream's close() method and clone it if it happens to be the
same instance before throwing it. But that's probably not a very clean
approach and it would only be effective for FilteredOuputStreams and
subclasses.

I think it would be better to just change the Throwable.addSuppressed()
method to treat self-suppression as a no-op. Or change javac to generate
code that only calls target.addSuppressed(t) when t != target ...

Regards, Peter

>
> -Alan.

[Alan Bateman]

On 05/05/2014 15:40, Peter Levart wrote:
>
> Hi Alan,
>
> There has been a discussion about a year ago on this list:
>
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-April/015947.html 
>
Yes, I remember this and also reviewed it for Joe. My point about it not
coming up before was in the context of BufferedWriter/etc where close
needs to flush. In that case then I think it would be more typically to
throw a new IOException each time.

I'll leave to Joe to comment on suggestion to drop self suppression.

-Alan

[Bernd Eckenfels-4]

Hello,

sorry for coming up with that thread again, but I see that JDK-8042377
is hanging around with no action, and there was not yet any discussion
I have seen about why it would be robust to call this IAE.

I know there was quite some work done in 2013 to make this
self-supression IAE a bit less painfull (8012044: Give more information
about self-suppression from Throwable.addSuppressed) but it was never
really discussed why it is needed at all?

Gruss
Bernd


 Am Mon, 05 May 2014 15:47:40 +0100
schrieb Alan Bateman <[hidden email]>:

> On 05/05/2014 15:40, Peter Levart wrote:
> >
> > Hi Alan,
> >
> > There has been a discussion about a year ago on this list:
> >
> > http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-April/015947.html 
> >
> Yes, I remember this and also reviewed it for Joe. My point about it
> not coming up before was in the context of BufferedWriter/etc where
> close needs to flush. In that case then I think it would be more
> typically to throw a new IOException each time.
>
> I'll leave to Joe to comment on suggestion to drop self suppression.
>
> -Alan