Code review request for 8012044: Give more information about self-suppression from Throwable.addSuppressed

[Joe Darcy]

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:

--- 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(SELF_SUPPRESSION_MESSAGE);
+            throw new 
IllegalArgumentException(SELF_SUPPRESSION_MESSAGE, exception);

          if (exception == null)
              throw new NullPointerException(NULL_CAUSE_MESSAGE);

Cheers,

-Joe

> Jason
>   
> Date: Fri, 12 Apr 2013 12:08:07 -0700
> From: joe.darcy at oracle.com
> To: jason_mehrens at hotmail.com
> CC: core-libs-dev at openjdk.java.net
> Subject: Re: Code review request for 8012044: Give more information about self-suppression from Throwable.addSuppressed
> On 04/12/2013 11:22 AM, Jason Mehrens wrote:
>
> The landmines are the retrofitted exception classes as shown here https://netbeans.org/bugzilla/show_bug.cgi?id=150969 and https://issues.jboss.org/browse/JBREM-552. Really, if the ISE or IAE is thrown it is going to suppress 'this' and 'cause'. It would be nice to see the given 'cause' show up in a log file when tracking down this type of bug.
> Okay; fair enough. Updated webrev covering initCause too at
>   http://cr.openjdk.java.net/~darcy/8012044.1/
>   New patch below.
>   (It is a bit of stretch to have this in initiCause by listed as the "cause" of the IllegalStateException; as an alternative, the IllegalStateException could have both this and the cause as suppressed exceptions.)
>   Cheers,
>   -Joe
>   --- old/src/share/classes/java/lang/Throwable.java 2013-04-12 12:03:48.000000000 -0700
>   +++ new/src/share/classes/java/lang/Throwable.java 2013-04-12 12:03:48.000000000 -0700
>   @@ -452,10 +452,14 @@
>   * @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);
>   + 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 +1043,7 @@
>   */
>   public final synchronized void addSuppressed(Throwable exception) {
>   if (exception == this)
>   - throw new IllegalArgumentException(SELF_SUPPRESSION_MESSAGE);
>   + throw new IllegalArgumentException(SELF_SUPPRESSION_MESSAGE, exception);
>   if (exception == null)
>   throw new NullPointerException(NULL_CAUSE_MESSAGE);
>   --- old/test/java/lang/Throwable/SuppressedExceptions.java 2013-04-12 12:03:49.000000000 -0700
>   +++ new/test/java/lang/Throwable/SuppressedExceptions.java 2013-04-12 12:03:48.000000000 -0700
>   @@ -26,7 +26,7 @@
>   /*
>   * @test
>   - * @bug 6911258 6962571 6963622 6991528 7005628
>   + * @bug 6911258 6962571 6963622 6991528 7005628 8012044
>   * @summary Basic tests of suppressed exceptions
>   * @author Joseph D. Darcy
>   */
>   @@ -40,6 +40,7 @@
>   serializationTest();
>   selfReference();
>   noModification();
>   + initCausePlumbing();
>   }
>   private static void noSelfSuppression() {
>   @@ -48,7 +49,9 @@
>   throwable.addSuppressed(throwable);
>   throw new RuntimeException("IllegalArgumentException for self-suppresion not thrown.");
>   } catch (IllegalArgumentException iae) {
>   - ; // Expected
>   + // Expected to be here
>   + if (iae.getCause() != throwable)
>   + throw new RuntimeException("Bad cause after self-suppresion.");
>   }
>   }
>   @@ -208,4 +211,29 @@
>   super("The medium.", null, enableSuppression, true);
>   }
>   }
>   +
>   + private static void initCausePlumbing() {
>   + Throwable t1 = new Throwable();
>   + Throwable t2 = new Throwable("message", t1);
>   + Throwable t3 = new Throwable();
>   +
>   + try {
>   + t2.initCause(t3);
>   + throw new RuntimeException("Shouldn't reach.");
>   + } catch (IllegalStateException ise) {
>   + if (ise.getCause() != t2)
>   + throw new RuntimeException("Unexpected cause in ISE", ise);
>   + Throwable[] suppressed = ise.getSuppressed();
>   + if (suppressed.length != 1 || suppressed[0] != t3)
>   + throw new RuntimeException("Bad suppression in ISE", ise);
>   + }
>   +
>   + try {
>   + t3.initCause(t3);
>   + throw new RuntimeException("Shouldn't reach.");
>   + } catch (IllegalArgumentException iae) {
>   + if (iae.getCause() != t3)
>   + throw new RuntimeException("Unexpected cause in ISE", iae);
>   + }
>   + }
>   }