Throwables.getRootCause leads to infinite loop sometimes

[GoogleCodeExporter]

Some SqlExceptions thrown by Oracle jdbc drivers has reference to itself in 
cause field. For this exception: ex == ex.getCause() and it leads to infinite 
loop if getRootCause() used.
Actually it can be treated as defect in drivers. Anyway, the proposed fix: 
additional condition could be used to prevent it.

  public static Throwable getRootCause(Throwable throwable) {
    Throwable cause;
    while ((cause = throwable.getCause()) != null && cause != throwable) {
      throwable = cause;
    }
    return throwable;
  }

Original issue reported on code.google.com by Alexandr...@gmail.com on 19 Oct 2012 at 6:36

Attachments:

• exception_chain.PNG

[GoogleCodeExporter]

That fix doesn't guard against cycles of size > 1 ;).

(No, this is not a serious complaint.)

Original comment by stephan...@gmail.com on 19 Oct 2012 at 7:58

[GoogleCodeExporter]

I can hardly imagine that :) Approach with slow/fast iterator will be better in 
this case.

Original comment by Alexandr...@gmail.com on 19 Oct 2012 at 8:09

[GoogleCodeExporter]

I don't know where exactly, but I seem to remember something similar to this 
being brought up before, and the conclusion I recall was that Throwables 
containing a loop in the causes are fundamentally broken (printStackTrace() 
will stack overflow, for example) and that we shouldn't do anything to try to 
accommodate them.

Original comment by cgdecker@google.com on 19 Oct 2012 at 5:04

• Added labels: Package-Base, Type-Defect

[GoogleCodeExporter]

Jdk's printStackTrace() uses IndentityHashSet named dejavu inside it to prevent 
such loops. 
I've implemented simple loop-detecting iterator - it works well with normal and 
end-looped cases and finds last element, but fails to find last element in case 
of cycles like 1->2->3->4->5->2. In latter case it returns some random element 
inside the cycle.
It can be reused in getCausalChain() which is affected to this issue too.
Approach with set of visited elements a much more heavier though it provide a 
better result.

Original comment by Alexandr...@gmail.com on 19 Oct 2012 at 6:57

Attachments:

• ThrowablesTest.java

[GoogleCodeExporter]

Interesting... that seems to be new in JDK7.

Original comment by cgdecker@google.com on 19 Oct 2012 at 8:25

[GoogleCodeExporter]

I wonder what should be the result in such a case. With loops of length 1 it's 
rather clear, but with longer ones?

I don't like the "slow/fast iterator" as it's a clever solution for cases when 
you can't afford the memory for the whole loop, which doesn't apply here. For 
maximum efficiency in the common case, you could follow `cause` (handling 
`cause==this` as `null`) for some bound number of iterations and switch to 
using dejaVu if it fails.

Original comment by Maaarti...@gmail.com on 19 Oct 2012 at 10:27

[GoogleCodeExporter]

Personally, I'd throw an exception as it's not possible to identify the root 
cause in this scenario. We might get clues, but if we do we're making 
assumptions on a certain behavior and I don't think Guava should go that way.

What I'd do is fix getCausalChain to detect loops and force people to use this 
one if they fear a loop might occur.

Original comment by ogregoire on 20 Oct 2012 at 1:01

[GoogleCodeExporter]

Well, I don't want to get any exception during getting root cause, I'd prefer 
to get last exception in terms of dejaVu - i.e. last unvisited one.
It can be implemented reasonable fast - bounded loop with fallback to dejaVu, 
or slow/fast with fallback to dejaVu, or even dejaVu itself - usage scenarios 
for me is error reporting, which is of course not a bottleneck :).

Original comment by Alexandr...@gmail.com on 20 Oct 2012 at 8:23

[GoogleCodeExporter]

That's exactly what I mean: you'd prefer to get the last unvisited one, but on 
my side, I have absolutelty no guarantee that it's the first in the whole 
chain. What you suggest is ambiguous.

Getting the whole chain is then possible by retrieving the last element 
returned by getCausalChain, which is be guaranteed to return what you'd expect. 
Here, the result is unequivocal.

Original comment by ogregoire on 21 Oct 2012 at 2:44

[GoogleCodeExporter]

We can get ambiguous result only in case when loop contains more than one 
element. I've never seen anything like this. Maybe throwing an exception is OK 
here. For one element loop the cause can be determined and returned.

Original comment by Alexandr...@gmail.com on 21 Oct 2012 at 5:41

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

"Some SqlExceptions thrown by Oracle jdbc drivers has reference to itself in 
cause field.": I think this is a red herring. By default, *every* Throwable has 
its |cause| field set to |this|. It's a special value that merely means 
"uninitialized." That's why getCause() looks like this...

    public synchronized Throwable getCause() {
        return (cause==this ? null : cause);
    }

...and initCause looks like this...

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

Notice in particular that getCause cannot return |this|.

None of this takes away from the larger point that it's possible to have cycles 
in the chain of causation. However, it doesn't sound like we have any reports 
that this is a problem in practice.

Original comment by cpov...@google.com on 29 Oct 2012 at 2:58

[GoogleCodeExporter]

Looked into jre (1.7) implementation - sometimes (see ClassNotFoundException 
for example) getCause() is overriden and returns value of private field w/o 
checks.

Original comment by Alexandr...@gmail.com on 29 Oct 2012 at 6:38

[GoogleCodeExporter]

Yes, and ClassNotFoundException requires that the cause be passed to the 
constructor. As a result, it can't be used to produce self-causation, either.

Original comment by cpov...@google.com on 29 Oct 2012 at 1:24

[GoogleCodeExporter]

Do we have any idea why some rare exceptions would need to have causative 
loops?  What made them do that in the first place?

Original comment by lowas...@google.com on 29 Oct 2012 at 2:32

[GoogleCodeExporter]

Unclear. The JDK bug that prompted their change has no details:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6962571

Original comment by cpov...@google.com on 29 Oct 2012 at 5:38

[GoogleCodeExporter]

Related: http://stackoverflow.com/questions/7585345/cycles-in-chained-exceptions

Original comment by brianfromoregon on 13 Nov 2012 at 1:46

[GoogleCodeExporter]

Someone could cause a similar infinite loop if they passed a cycling iterable 
to an immutable copyOf() method. These bugs should be easy to detect and catch 
and really aren't worth additional code, complexity, and runtime cost to 
"normal" users.

Original comment by kak@google.com on 22 Aug 2013 at 10:55

• Changed state: WorkingAsIntended

[GoogleCodeExporter]

This issue has been migrated to GitHub.

It can be found at https://github.com/google/guava/issues/<id>

Original comment by cgdecker@google.com on 1 Nov 2014 at 4:13

• Added labels: MigratedToGitHub

[GoogleCodeExporter]

Original comment by cgdecker@google.com on 3 Nov 2014 at 9:08

• Added labels: Restrict-AddIssueComment-Commit