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

[David Holmes]

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

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

The suppression mechanism is typically, but not exclusively, used by
the try-with-resources statement.

Thanks,

-Joe