REASSERT Code review request for 8012044: Give more information about self-suppression from Throwable.addSuppressed (original) (raw)

David Holmes david.holmes at oracle.com
Tue Apr 30 06:38:33 UTC 2013

Sorry this slipped through the cracks.

Looks good to me. (Don't know if you already pushed it :) )

David

On 25/04/2013 5:16 PM, Joe Darcy wrote:

Hello,

Responding to David's comment and some comments from Alan off-list, here is a variant which doesn't use suppressed exceptions in initCause, but still passes along some information: http://cr.openjdk.java.net/~darcy/8012044.4 Patch to Throwable: --- a/src/share/classes/java/lang/Throwable.java Wed Apr 24 21:27:52 2013 +0000 +++ b/src/share/classes/java/lang/Throwable.java Thu Apr 25 00:15:32 2013 -0700 @@ -453,9 +453,10 @@ */ public synchronized Throwable initCause(Throwable cause) { if (this.cause != this) - throw new IllegalStateException("Can't overwrite cause"); + throw new IllegalStateException("Can't overwrite cause with " + + Objects.toString(cause, "a null"), this); if (cause == this) - throw new IllegalArgumentException("Self-causation not permitted"); + throw new IllegalArgumentException("Self-causation not permitted", this); this.cause = cause; return this; } @@ -1039,7 +1040,7 @@ */ public final synchronized void addSuppressed(Throwable exception) { if (exception == this) - throw new IllegalArgumentException(SELFSUPPRESSIONMESSAGE); + throw new IllegalArgumentException(SELFSUPPRESSIONMESSAGE, exception); if (exception == null) throw new NullPointerException(NULLCAUSEMESSAGE); Thanks, -Joe On 04/22/2013 10:51 PM, Joe Darcy wrote: Hi David,

On 04/22/2013 10:25 PM, David Holmes wrote: Hi Joe,

On 23/04/2013 9:05 AM, Joseph Darcy wrote: Hello,

Just reasserting the request for a review of the latest version of this patch: http://cr.openjdk.java.net/~darcy/8012044.2 I believe this version does an appropriate job of propagating exception information when there is misuse of the methods on Throwable. I still find the use of addSuppressed in initCause to be questionable. Given: catch(SomeException s) { sharedException.initCause(s); // oops already has a cause throw sharedException; } then the ISE isn't suppressing 's', but replacing/suppressing sharedException in my view, as it is what would have been thrown had the ISE not occurred. I understand the desire to not lose sight of the fact that 's' was thrown, but this is really no different to a finally block losing the original exception info. (And fixing that was rejected when the suppression mechanism was added.) Project Coin discussions did note try-catch-finally and try-with-resources were inconsistent on this point. While the try-with-resources behavior is regarded as preferable, we thought it would be too large a change to redefine the long-standing semantics of try-catch-finally. Anyway this isn't a "block" (not that such a thing exists), just a comment. The change isn't harmful and may be useful. Cheers, David Yes, I would describe the intention of this change as provding programmers more information to debug when the methods are Throwable and used improperly. Thanks, -Joe

More information about the core-libs-dev mailing list