Quantcast

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

classic Classic list List threaded Threaded
22 messages Options
12
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Joe Darcy
Hello,

Please review the patch below to address

     8012044: Give more information about self-suppression from
Throwable.addSuppressed
     http://cr.openjdk.java.net/~darcy/8012044.0/

Thanks,

-Joe

diff -r 006a7a576fe9 src/share/classes/java/lang/Throwable.java
--- a/src/share/classes/java/lang/Throwable.java    Thu Apr 11 12:22:23
2013 +0900
+++ b/src/share/classes/java/lang/Throwable.java    Thu Apr 11 18:16:38
2013 -0700
@@ -1039,7 +1039,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);
diff -r 006a7a576fe9 test/java/lang/Throwable/SuppressedExceptions.java
--- a/test/java/lang/Throwable/SuppressedExceptions.java    Thu Apr 11
12:22:23 2013 +0900
+++ b/test/java/lang/Throwable/SuppressedExceptions.java    Thu Apr 11
18:16:38 2013 -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
   */
@@ -48,7 +48,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.");
          }
      }


-Joe
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Zhong Yu
It doesn't feel very right to say the exception is the "cause" of the IAE.

If the user code reuses an exception instance "x", as reported by OP,
there is a possibility that the IAE is later added as a suppressed
exception to "x", forming a loop x->IAE->x.

Zhong Yu


On Thu, Apr 11, 2013 at 8:19 PM, Joe Darcy <[hidden email]> wrote:

> Hello,
>
> Please review the patch below to address
>
>     8012044: Give more information about self-suppression from
> Throwable.addSuppressed
>     http://cr.openjdk.java.net/~darcy/8012044.0/
>
> Thanks,
>
> -Joe
>
> diff -r 006a7a576fe9 src/share/classes/java/lang/Throwable.java
> --- a/src/share/classes/java/lang/Throwable.java    Thu Apr 11 12:22:23 2013
> +0900
> +++ b/src/share/classes/java/lang/Throwable.java    Thu Apr 11 18:16:38 2013
> -0700
> @@ -1039,7 +1039,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);
> diff -r 006a7a576fe9 test/java/lang/Throwable/SuppressedExceptions.java
> --- a/test/java/lang/Throwable/SuppressedExceptions.java    Thu Apr 11
> 12:22:23 2013 +0900
> +++ b/test/java/lang/Throwable/SuppressedExceptions.java    Thu Apr 11
> 18:16:38 2013 -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
>   */
> @@ -48,7 +48,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.");
>          }
>      }
>
>
> -Joe
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Peter Levart
In reply to this post by Joe Darcy
Hi Joe,

There were certainly debates about why self-suppression is not a good
thing when project Coin's try-with-resources has been developed, but I
don't quite remember the reason why it was designed this way. Couldn't
the logic just make the self-suppression a no-op? The addSuppressed was
designed for (quoting from javadoc):

"...there are situations where two independent exceptions can be thrown
in sibling code blocks, in particular in the try block of a
try-with-resources statement and the compiler-generated finally block
which closes the resource. In these situations, only one of the thrown
exceptions can be propagated..."

Now if those "two independent exceptions" are actually the same
exception, there's no "dilemma"...


Regards, Peter


On 04/12/2013 03:19 AM, Joe Darcy wrote:

> Hello,
>
> Please review the patch below to address
>
>     8012044: Give more information about self-suppression from
> Throwable.addSuppressed
>     http://cr.openjdk.java.net/~darcy/8012044.0/
>
> Thanks,
>
> -Joe
>
> diff -r 006a7a576fe9 src/share/classes/java/lang/Throwable.java
> --- a/src/share/classes/java/lang/Throwable.java    Thu Apr 11
> 12:22:23 2013 +0900
> +++ b/src/share/classes/java/lang/Throwable.java    Thu Apr 11
> 18:16:38 2013 -0700
> @@ -1039,7 +1039,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);
> diff -r 006a7a576fe9 test/java/lang/Throwable/SuppressedExceptions.java
> --- a/test/java/lang/Throwable/SuppressedExceptions.java    Thu Apr 11
> 12:22:23 2013 +0900
> +++ b/test/java/lang/Throwable/SuppressedExceptions.java    Thu Apr 11
> 18:16:38 2013 -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
>   */
> @@ -48,7 +48,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.");
>          }
>      }
>
>
> -Joe

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Jason Mehrens
In reply to this post by Joe Darcy
Joe, Should this same logic be applied to the exceptions thrown from initCause?  Seems like that would be consistent with this change. Jason
 > Date: Thu, 11 Apr 2013 18:19:30 -0700

> From: [hidden email]
> To: [hidden email]
> Subject: Code review request for 8012044: Give more information about self-suppression from Throwable.addSuppressed
>
> Hello,
>
> Please review the patch below to address
>
>      8012044: Give more information about self-suppression from
> Throwable.addSuppressed
>      http://cr.openjdk.java.net/~darcy/8012044.0/
>
> Thanks,
>
> -Joe
>
> diff -r 006a7a576fe9 src/share/classes/java/lang/Throwable.java
> --- a/src/share/classes/java/lang/Throwable.java    Thu Apr 11 12:22:23
> 2013 +0900
> +++ b/src/share/classes/java/lang/Throwable.java    Thu Apr 11 18:16:38
> 2013 -0700
> @@ -1039,7 +1039,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);
> diff -r 006a7a576fe9 test/java/lang/Throwable/SuppressedExceptions.java
> --- a/test/java/lang/Throwable/SuppressedExceptions.java    Thu Apr 11
> 12:22:23 2013 +0900
> +++ b/test/java/lang/Throwable/SuppressedExceptions.java    Thu Apr 11
> 18:16:38 2013 -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
>    */
> @@ -48,7 +48,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.");
>           }
>       }
>
>
> -Joe
     
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Joe Darcy
Hi Jason,

Hmm. This is the current initCause implementation from JDK 8:

     public synchronized Throwable initCause(Throwable cause) {
         if (this.cause != this)
             throw new IllegalStateException("Can't overwrite cause");
         if (cause == this)
             throw new IllegalArgumentException("Self-causation not
permitted");
         this.cause = cause;
         return this;
     }

It wouldn't be unreasonable to change the second throw to

         if (cause == this)
             throw new IllegalArgumentException("Self-causation not
permitted", cause);

but I think there is less motivation to do so than in the addSuppressed
case since addSuppressed gets called in compiler-generated code that
isn't visible in the original sources.

-Joe

On 04/12/2013 05:45 AM, Jason Mehrens wrote:

> Joe,
>
> Should this same logic be applied to the exceptions thrown from
> initCause?  Seems like that would be consistent with this change.
>
> Jason
>
> > Date: Thu, 11 Apr 2013 18:19:30 -0700
> > From: [hidden email]
> > To: [hidden email]
> > Subject: Code review request for 8012044: Give more information
> about self-suppression from Throwable.addSuppressed
> >
> > Hello,
> >
> > Please review the patch below to address
> >
> > 8012044: Give more information about self-suppression from
> > Throwable.addSuppressed
> > http://cr.openjdk.java.net/~darcy/8012044.0/
> >
> > Thanks,
> >
> > -Joe
> >
> > diff -r 006a7a576fe9 src/share/classes/java/lang/Throwable.java
> > --- a/src/share/classes/java/lang/Throwable.java    Thu Apr 11 12:22:23
> > 2013 +0900
> > +++ b/src/share/classes/java/lang/Throwable.java    Thu Apr 11 18:16:38
> > 2013 -0700
> > @@ -1039,7 +1039,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);
> > diff -r 006a7a576fe9 test/java/lang/Throwable/SuppressedExceptions.java
> > --- a/test/java/lang/Throwable/SuppressedExceptions.java    Thu Apr 11
> > 12:22:23 2013 +0900
> > +++ b/test/java/lang/Throwable/SuppressedExceptions.java    Thu Apr 11
> > 18:16:38 2013 -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
> > */
> > @@ -48,7 +48,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.");
> > }
> > }
> >
> >
> > -Joe

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Jason Mehrens
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. Date: Fri, 12 Apr 2013 10:35:40 -0700
From: [hidden email]
To: [hidden email]
CC: [hidden email]
Subject: Re: Code review request for 8012044: Give more information about self-suppression from Throwable.addSuppressed


 
   
 
 
    Hi Jason,

     

      Hmm. This is the current initCause implementation from JDK 8:

     

          public synchronized Throwable initCause(Throwable cause) {

              if (this.cause != this)

                  throw new IllegalStateException("Can't overwrite
      cause");

              if (cause == this)

                  throw new IllegalArgumentException("Self-causation not
      permitted");

              this.cause = cause;

              return this;

          }

     

      It wouldn't be unreasonable to change the second throw to

     

              if (cause == this)

                  throw new IllegalArgumentException("Self-causation not
      permitted", cause);

     

      but I think there is less motivation to do so than in the
      addSuppressed case since addSuppressed gets called in
      compiler-generated code that isn't visible in the original
      sources.

     

      -Joe



             
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Jason Mehrens
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) {
     }
}

Jason
 
Date: Fri, 12 Apr 2013 12:08:07 -0700
From: [hidden email]
To: [hidden email]
CC: [hidden email]
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);
 + }
 + }
 }    
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: 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: [hidden email]
> To: [hidden email]
> CC: [hidden email]
> 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);
>   + }
>   + }
>   }  

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

David Holmes
In reply to this post by Joe Darcy
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);
> +        }
> +    }
>   }
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Jason Mehrens
David,

The last two paragraphs of Throwable.addSuppressed cover this.  The first is your argument bellow and does not forbid anything it claims.  The last paragraph explicitly enables this patch.

Jason

> Date: Mon, 15 Apr 2013 12:56:42 +1000
> From: [hidden email]
> To: [hidden email]
> CC: [hidden email]; [hidden email]
> Subject: Re: Code review request for 8012044: Give more information about self-suppression from Throwable.addSuppressed
>
> 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    
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Joe Darcy
In reply to this post by Joe Darcy
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
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Jason Mehrens
Joe,

The patch at http://cr.openjdk.java.net/~darcy/8012044.3/ looks good to me.  Thanks for working on this.

Jason
 
----------------------------------------

> Date: Wed, 17 Apr 2013 10:32:10 -0700
> From: [hidden email]
> To: [hidden email]
> CC: [hidden email]; [hidden email]
> Subject: Re: Code review request for 8012044: Give more information about self-suppression from Throwable.addSuppressed
>
> 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    
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Joe Darcy
In reply to this post by Joe Darcy
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.

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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Joe Darcy
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
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Remi Forax
On 04/23/2013 07:51 AM, 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

Like David,
I think that the use of addSuppressed is a bit too much,
suppressed exception are exceptions that were thrown and there is no
guarantee that a cause was thrown before
(it's sometime the case, but sometimes the cause is used as a
'tunelling' mechanism,
If I want to throw ThatException but the method only throw ThisException
so I will create a ThisException
that used a newly created ThatException as cause. In that case, the
cause was never thrown
and register it has a suppressed exception is weird IMO.

cheers,
RĂ©mi



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Jason Mehrens
In reply to this post by David Holmes
> 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 agree. I think makes sense to swap the ISE cause and suppressed arguments because the root cause should be the most important throwable to see in a log file.
In David's example, that would be 's' or the root cause of 's'. Also when the two arguments are swapped the line numbers are in descending order.
 
===========JDK 7 testcase===========================
public static void main(String[] args) throws Exception {
 final Throwable cause = new NoClassDefFoundError();
 final Throwable THIS = new ClassNotFoundException();
 try {
            THIS.initCause(cause); //It's a trap!!
 } catch (IllegalStateException ISE) {
            ISE.initCause(cause); //swapped
            ISE.addSuppressed(THIS); //swapped
            ISE.printStackTrace();
 }
 }
 
java.lang.IllegalStateException: Can't overwrite cause
 at java.lang.Throwable.initCause(Throwable.java:456)
 at Main.main(Main.java:33)
 Suppressed: java.lang.ClassNotFoundException
  at Main.main(Main.java:31)
Caused by: java.lang.NoClassDefFoundError
 at Main.main(Main.java:30)
===============================================
 
Jason    
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Joe Darcy
In reply to this post by 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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Alan Bateman
On 25/04/2013 08:16, Joe Darcy wrote:
> 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
>
This looks reasonable to me in that it provides useful information in
the ISA. Like David, I would have found it confusing to have the cause
show up as a suppressed exception in output.

-Alan.
12
Loading...