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

[Joe Darcy]

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

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

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