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

[David Holmes]

On 13/04/2013 5:08 AM, Joe Darcy wrote:
> 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.)

I don't think that is valid for initCause. If I call initCause twice in 
succession there need not even be any exception propagation in progress. 
The ISE that gets thrown is not suppressing anything.

For setSuppressed this might make sense for the compiler generated 
sequences, but again if I just called setSuppressed with an invalid 
value, the ISE is not suppressing any existing exception.

David
-----

> 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);
> +        }
> +    }
>   }
>