Trimming exceptions in IgniteUtils::cast

[Stanislav Lukyanov]

Hi,

IgniteUtils::cast method is used to cast an exception to IgniteCheckedException. It is done by trimming the top exceptions in the trace until we find IgniteCheckedException, or until we’ve found the last one. As a result, IgniteUtils::cast generally can trim the whole stack trace but the root exception.

One problem caused by that is https://issues.apache.org/jira/browse/IGNITE-7904.
ComputeTask::result may throw IgniteException which is to be rethrown by the ComputeTaskFuture::get, but in fact the IgniteException and all its causes (but the root one) are trimmed, and the exception that comes from the ComputeTaskFuture::get is different than excepted.

To fix that I want to change the IgniteUtils::cast not to remove any exceptions from the stack trace and just wrap all arguments with IgniteCheckException.
Of course, this will affect all places where IgniteUtils::cast is used (mostly various futures completion).
Does anyone have concerns about this?

To illustrate, a patch (untested!) for IgniteUtils is below.

Thanks,
Stan

diff --git a/modules/core/src/main/java/org/apache/ignite/internal/util/IgniteUtils.java b/modules/core/src/main/java/org/apache/ignite/internal/util/IgniteUtils.java
index cbe64cd97af..5e8d9c8f4d9 100755
--- a/modules/core/src/main/java/org/apache/ignite/internal/util/IgniteUtils.java
+++ b/modules/core/src/main/java/org/apache/ignite/internal/util/IgniteUtils.java
@@ -7256,26 +7256,16 @@ public abstract class IgniteUtils {
     public static IgniteCheckedException cast(Throwable t) {
         assert t != null;
 
-        while (true) {
-            if (t instanceof Error)
-                throw (Error)t;
+        while (t instanceof GridClosureException)
+            t = ((GridClosureException)t).unwrap();
 
-            if (t instanceof GridClosureException) {
-                t = ((GridClosureException)t).unwrap();
+        if (t instanceof Error)
+            throw (Error)t;
 
-                continue;
-            }
-
-            if (t instanceof IgniteCheckedException)
-                return (IgniteCheckedException)t;
-
-            if (!(t instanceof IgniteException) || t.getCause() == null)
-                return new IgniteCheckedException(t);
+        if (t instanceof IgniteCheckedException)
+            return (IgniteCheckedException)t;
 
-            assert t.getCause() != null; // ...and it is IgniteException.
-
-            t = t.getCause();
-        }
+        return new IgniteCheckedException(t);
     }
 

[Valentin Kulichenko]

Generally, I think we should not trim any exceptions, because this way we
can unexpectedly remove useful information. Do we know what was the
original reasoning behind this logic?

-Val

On Mon, Mar 12, 2018 at 3:57 AM, Stanislav Lukyanov <[hidden email]>
wrote:

> Hi,
>
> IgniteUtils::cast method is used to cast an exception to
> IgniteCheckedException. It is done by trimming the top exceptions in the
> trace until we find IgniteCheckedException, or until we’ve found the last
> one. As a result, IgniteUtils::cast generally can trim the whole stack
> trace but the root exception.
>
> One problem caused by that is https://issues.apache.org/
> jira/browse/IGNITE-7904.
> ComputeTask::result may throw IgniteException which is to be rethrown by
> the ComputeTaskFuture::get, but in fact the IgniteException and all its
> causes (but the root one) are trimmed, and the exception that comes from
> the ComputeTaskFuture::get is different than excepted.
>
> To fix that I want to change the IgniteUtils::cast not to remove any
> exceptions from the stack trace and just wrap all arguments with
> IgniteCheckException.
> Of course, this will affect all places where IgniteUtils::cast is used
> (mostly various futures completion).
> Does anyone have concerns about this?
>
> To illustrate, a patch (untested!) for IgniteUtils is below.
>
> Thanks,
> Stan
>
> diff --git a/modules/core/src/main/java/org/apache/ignite/internal/util/IgniteUtils.java
> b/modules/core/src/main/java/org/apache/ignite/internal/
> util/IgniteUtils.java
> index cbe64cd97af..5e8d9c8f4d9 100755
> --- a/modules/core/src/main/java/org/apache/ignite/internal/
> util/IgniteUtils.java
> +++ b/modules/core/src/main/java/org/apache/ignite/internal/
> util/IgniteUtils.java
> @@ -7256,26 +7256,16 @@ public abstract class IgniteUtils {
>      public static IgniteCheckedException cast(Throwable t) {
>          assert t != null;
>
> -        while (true) {
> -            if (t instanceof Error)
> -                throw (Error)t;
> +        while (t instanceof GridClosureException)
> +            t = ((GridClosureException)t).unwrap();
>
> -            if (t instanceof GridClosureException) {
> -                t = ((GridClosureException)t).unwrap();
> +        if (t instanceof Error)
> +            throw (Error)t;
>
> -                continue;
> -            }
> -
> -            if (t instanceof IgniteCheckedException)
> -                return (IgniteCheckedException)t;
> -
> -            if (!(t instanceof IgniteException) || t.getCause() == null)
> -                return new IgniteCheckedException(t);
> +        if (t instanceof IgniteCheckedException)
> +            return (IgniteCheckedException)t;
>
> -            assert t.getCause() != null; // ...and it is IgniteException.
> -
> -            t = t.getCause();
> -        }
> +        return new IgniteCheckedException(t);
>      }
>
>

[Alexey Goncharuk]

Stanislav,

Thanks for the patch. It is a bit complex change, I will try to review it
over the next weekend.

--AG

2018-03-12 21:34 GMT+03:00 Valentin Kulichenko <
[hidden email]>:

> Generally, I think we should not trim any exceptions, because this way we
> can unexpectedly remove useful information. Do we know what was the
> original reasoning behind this logic?
>
> -Val
>
> On Mon, Mar 12, 2018 at 3:57 AM, Stanislav Lukyanov <
> [hidden email]>
> wrote:
>
> > Hi,
> >
> > IgniteUtils::cast method is used to cast an exception to
> > IgniteCheckedException. It is done by trimming the top exceptions in the
> > trace until we find IgniteCheckedException, or until we’ve found the last
> > one. As a result, IgniteUtils::cast generally can trim the whole stack
> > trace but the root exception.
> >
> > One problem caused by that is https://issues.apache.org/
> > jira/browse/IGNITE-7904.
> > ComputeTask::result may throw IgniteException which is to be rethrown by
> > the ComputeTaskFuture::get, but in fact the IgniteException and all its
> > causes (but the root one) are trimmed, and the exception that comes from
> > the ComputeTaskFuture::get is different than excepted.
> >
> > To fix that I want to change the IgniteUtils::cast not to remove any
> > exceptions from the stack trace and just wrap all arguments with
> > IgniteCheckException.
> > Of course, this will affect all places where IgniteUtils::cast is used
> > (mostly various futures completion).
> > Does anyone have concerns about this?
> >
> > To illustrate, a patch (untested!) for IgniteUtils is below.
> >
> > Thanks,
> > Stan
> >
> > diff --git a/modules/core/src/main/java/org/apache/ignite/internal/
> util/IgniteUtils.java
> > b/modules/core/src/main/java/org/apache/ignite/internal/
> > util/IgniteUtils.java
> > index cbe64cd97af..5e8d9c8f4d9 100755
> > --- a/modules/core/src/main/java/org/apache/ignite/internal/
> > util/IgniteUtils.java
> > +++ b/modules/core/src/main/java/org/apache/ignite/internal/
> > util/IgniteUtils.java
> > @@ -7256,26 +7256,16 @@ public abstract class IgniteUtils {
> >      public static IgniteCheckedException cast(Throwable t) {
> >          assert t != null;
> >
> > -        while (true) {
> > -            if (t instanceof Error)
> > -                throw (Error)t;
> > +        while (t instanceof GridClosureException)
> > +            t = ((GridClosureException)t).unwrap();
> >
> > -            if (t instanceof GridClosureException) {
> > -                t = ((GridClosureException)t).unwrap();
> > +        if (t instanceof Error)
> > +            throw (Error)t;
> >
> > -                continue;
> > -            }
> > -
> > -            if (t instanceof IgniteCheckedException)
> > -                return (IgniteCheckedException)t;
> > -
> > -            if (!(t instanceof IgniteException) || t.getCause() == null)
> > -                return new IgniteCheckedException(t);
> > +        if (t instanceof IgniteCheckedException)
> > +            return (IgniteCheckedException)t;
> >
> > -            assert t.getCause() != null; // ...and it is
> IgniteException.
> > -
> > -            t = t.getCause();
> > -        }
> > +        return new IgniteCheckedException(t);
> >      }
> >
> >
>