Failure.get should wrap exception before throwing

[scabug]

Otherwise, the stacktrace of the code calling Failure.get is lost. Wrapping the Failure exception preserves both stacktraces.

This is important because if Failure.get is called this is likely to be a bug in the code that calls Failure.get (and not in the code that produced the original Failure).

For some usages of Try this may not be too big a problem because Failure creation and calling Failure.get may occur together so that it is easy enough to infer where the problem is. However, in many cases Try is used to transport a possibly erroneous result over "bigger distances" where original stacktrace and Failure.get aren't executed close to each other so that finding the cause can get hard.

One particularly irritating effect of the current implementation is that Await.result(future) directly relies on Try.get so that the exception being thrown by that call doesn't include the stacktrace of the Await.result call but just the mostly useless stacktrace of the failed Future execution (e.g. a "Future timed out" stack trace that doesn't include any user code).

[scabug]

Imported From: https://issues.scala-lang.org/browse/SI-8963?orig=1
Reporter: @jrudolph
Affected Versions: 2.10.4, 2.11.4

[scabug]

@jrudolph said:
I didn't mention that this is a breaking change because any code relying on Try.get throwing the original exception will break.

My reasoning why this change is still a net-improvement is that you can always match on an expression of type Try[T] to get out the original error message if you want to. Relying on try/catch to get failures out of a Try should be considered an anti-pattern anyways.

Calling Try.get itself is an anti-pattern, but again on the other hand recovering the stacktrace of the thread calling Try.get / Await.result is impossible if the call is buried inside any third-party library and then debugging is next to impossible.

[scabug]

@jrudolph said:
Here's a (artificial but very similar to the actual) scenario: How would you debug this stacktrace from a log?

Caused by: akka.pattern.AskTimeoutException: Ask timed out on [Actor[akka://default/user/handler]] after [500 ms]
 	at akka.pattern.PromiseActorRef$$anonfun$1.apply$mcV$sp(AskSupport.scala:333)
 	at akka.actor.Scheduler$$anon$7.run(Scheduler.scala:117)
 	at scala.concurrent.Future$InternalCallbackExecutor$.unbatchedExecute(Future.scala:599)
 	at scala.concurrent.BatchingExecutor$class.execute(BatchingExecutor.scala:109)
 	at scala.concurrent.Future$InternalCallbackExecutor$.execute(Future.scala:597)
 	at akka.actor.LightArrayRevolverScheduler$TaskHolder.executeTask(Scheduler.scala:467)
 	at akka.actor.LightArrayRevolverScheduler$$anon$8.executeBucket$1(Scheduler.scala:419)
 	at akka.actor.LightArrayRevolverScheduler$$anon$8.nextTick(Scheduler.scala:423)
 	at akka.actor.LightArrayRevolverScheduler$$anon$8.run(Scheduler.scala:375)
 	at java.lang.Thread.run(Thread.java:745)

My code doesn't use the ask pattern and my code doesn't use Await (but of course, any third party code I depend on may, I don't know). It only happens on production where I cannot attach a debugger.

If Failure.get would provide a proper stacktrace finding the problem may have been very simple. Or at least working around it, as it would show where the problem is raised in my own code.

[scabug]

@retronym said (edited on Nov 6, 2014 1:15:16 PM UTC):
I don't think we can break the invariant that:

def prop(t: Throwable): Boolean = try { Try(throw t).get } catch t1: Throwable => t1 eq t }

However we might consider using Throwable#setStackTrace to augment the stack trace. We'd need to find other frameworks/libraries that use that to understand if it is possible/advisable.

[scabug]

@jrudolph said:
Why? Would you also suggest this invariant:

def prop(t: Throwable): Boolean = try { Try(throw t) match { case Success(t) => t }  } catch t1: Throwable => t1 eq t }

[scabug]

@jrudolph said:
A precedent: java.util.concurrent.Future.get wraps an exception that occurred during running a Future into an ExecutionException. http://docs.oracle.com/javase/7/docs/api/java/util/concurrent/Future.html#get()

[scabug]

@retronym said:
As I understand it, Try is meant to be a library equivalent of the built in try. It is a way to transport an exception across a thread boundary. Once you have the exception on your thread, you should be able to pattern match against as you would have in the catch clauses, without having to remember to call .getCause first. Existing code will rely on this property, so IMO we can't change this other than by introduce a new variant of get that wraps and captures the additional stack of the current thread.

Java's use of wrapper exceptions may also be motivated by Java's checked exceptions.

[scabug]

@jrudolph said:
First, I agree that using setStackTrace could be a solution (though a non-standard one AFAIK). Also, yes, checked exceptions are probably the main reasons for the wrapper exceptions. Still, that means that wrapper exceptions are not a new thing.

But stepping back a bit, maybe let's focus on the problem. My basic problem is that Try.get doesn't work consistently with Option.get or Either.right.get and that was what I expected (and I guess other people as well). I regard all of the get methods as a shortcut for an assertion and a cast:

val x: Option[Int]
x.get
// = 
// assert(x.isDefined, "programmer assumed that x.isDefined")
// x.asInstanceOf[Some].value

Here's now an example showing the inconsistency:

  def optionTest(x: Option[Int]): Unit = {
    x.get // oops, unguarded access, bug in this line

    // Exception points to the buggy line:

    // java.util.NoSuchElementException: None.get
    //   at scala.None$.get(Option.scala:313)
    //   at scala.None$.get(Option.scala:311)
    //   at example.GetInconsistence$.optionTest(GetInconsistence.scala:6)
  }
  def eitherTest(x: Either[String, Int]): Unit = {
    x.right.get // oops, unguarded access, bug in this line

    // Exception points to the buggy line:

    // java.util.NoSuchElementException: Either.right.value on Left
    // 	at scala.util.Either$RightProjection.get(Either.scala:454)
    // 	at example.GetInconsistence$.eitherTest(GetInconsistence.scala:17)
  }
  def tryTest(x: Try[Int]): Unit = {
    x.get // oops, unguarded access, bug in this line

    // Exception doesn't include buggy line at all:

    // 	at example.Service$$anonfun$tryResult$1.apply(GetInconsistence.scala:9)
    //  at example.Service$$anonfun$tryResult$1.apply(GetInconsistence.scala:9)
    // 	at scala.util.Try$.apply(Try.scala:161)
    //  at example.Service$.tryResult(GetInconsistence.scala:9)
    // 	at example.GetInconsistence$$anonfun$3.apply$mcV$sp(GetInconsistence.scala:50)
  }

If we classify this inconsistency as a problem introducing a new method with new semantics won't solve the problem: users may still assume consistency and use the old method as before.

IMO the long-term solution could be:

• Failure.get wraps the exception
• there's a new method Try.getOrRethrow that mimics the current semantics for everyone who relies on it

I don't suggest wrapping the exception in any case, you can still pattern match on Try and get the original message out of the Failure object as before, I just suggest to change the semantics of Failure.get in the long term.

How to deal with the compatibility problems is another topic that needs to be solved but maybe we should agree on the problem first :)