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 23 05:25:34 UTC 2013

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.)

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

Thanks,

-Joe On 4/17/2013 10:32 AM, Joe Darcy wrote: On 04/14/2013 07:36 PM, Joe Darcy wrote:

On 04/12/2013 07:29 PM, Jason Mehrens wrote:

Joe, You'll have guard ise.addSuppressed against null. Looks good otherwise.

private static void initCauseNull() { Throwable t1 = new Throwable(); t1.initCause(null); try { t1.initCause(null); } catch(IllegalStateException expect) { } } Right you are; check added and test updated in: http://cr.openjdk.java.net/~darcy/8012044.2/ Full patch to Throwable: [snip] One more iteration; I've changed the initCause logic to suppress both exceptions rather than using one as the cause: http://cr.openjdk.java.net/~darcy/8012044.2 Patch to throwable: --- old/src/share/classes/java/lang/Throwable.java 2013-04-14 19:33:23.000000000 -0700 +++ new/src/share/classes/java/lang/Throwable.java 2013-04-14 19:33:23.000000000 -0700 @@ -452,10 +452,15 @@ * @since 1.4 */ public synchronized Throwable initCause(Throwable cause) { - if (this.cause != this) - throw new IllegalStateException("Can't overwrite cause"); + if (this.cause != this) { + IllegalStateException ise = + new IllegalStateException("Can't overwrite cause", this); + if (cause != null) + ise.addSuppressed(cause); + throw ise; + } 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 +1044,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); The suppression mechanism is typically, but not exclusively, used by the try-with-resources statement. Thanks, -Joe

More information about the core-libs-dev mailing list