FilterOutputStream.close() throws exception from flush()
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; } }
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.
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-suppressi... Bernd Am 05.05.2014 11:32 schrieb "Alan Bateman" <Alan.Bateman@oracle.com>:
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.
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.
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
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 <Alan.Bateman@oracle.com>:
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
participants (4)
-
Alan Bateman
-
Bernd
-
Bernd Eckenfels
-
Peter Levart