Add the ability to handle caught async exceptions in zones

[nex3]

Right now there are only two ways to use a ZoneSpecification to interact with exceptions: [handleUncaughtError], which is passed the error when it would leave the zone; and [registerCallback] et al, which can wrap the callback in a try/catch block to capture exceptions that are raised.

I'm trying to implement a solution to issue #7040 using zones (as suggested on that bug), and I'm finding that these two methods of interacting with exceptions aren't sufficient. When tracking the stack traces that are relevant to a given exception, it's important to know the context at the exact point in time the exception is signaled. We can do this to some extent using [registerCallback] and try/catch, but this provides no visibility into exceptions reported using e.g. [Completer.completeError].

Here's a straw-man proposal: ZoneSpecification.registerErrorHandler takes the same arguments as HandleUncaughtErrorHandler and returns something like the internal _AsyncError type -- an error/stack-trace pair. It's called whenever an exception reaches dart:async, either by being thrown or passed explicitly using [Completer.completeError] or similar. The return value is used as the error that's saved and propagated, or passed to [handleUncaughtError].

In addition to supporting issue #7040, this would make it easy to e.g. write code to detect places where errors are thrown without stack traces, or even to automatically add stack traces.

[floitschG]

Agreed. This is something we/I didn't think of.
We are discussing our options.

---

Removed Type-Defect label.
Added Type-Enhancement label.

[nex3]

Has there been any movement on this? We're looking at moving pub over to Kevin and Eric's async/await prototype, which means our workaround will stop working.

[floitschG]

I would rather try to discourage the use of completers. In almost all cases (except for native interaction) they shouldn't be necessary.
Especially now with async/await it should be more natural to just write "throw x".

[nex3]

That's not good enough. You can discourage all you want, but they're still going to get used, and it's important that the stack_chain package be able to track all exceptions, not just the ones that are surfaced using nice-looking code.

There are also plenty of cases where completers will still need to be used for complex asynchronous operators and classes such as FutureGroup. [new Future] and [new Future.sync] also produce exceptions that are currently invisible to zones, as does almost every asynchronous dart:io function.

[floitschG]

We should investigate if we can fix new Future and new Future.sync.

---

cc @lrhn.

[nex3]

Adding this capability to zones would fix them :).

You seemed in favor of this when I first posted it. What changed?

[floitschG]

Mainly: I haven't found a good way to do it.

[nex3]

Couldn't you add something like

    dynamic handleError(error, StackTrace stackTrace)

to Zone (which would return a new error object), and have that be called by [Completer.completeError], [StreamController.addError], and the various Future methods and constructors that catch and wrap errors?

If you wanted, you could even have it return both an error and a stack trace somehow, which would make the stack chain stuff really nice.

[nex3]

As we transition pub to use Kevin and Erik's async_await package, fixing this is becoming more and more critical. That package generates a lot of completers and raw calls to Future methods, which means the Chain.track workaround doesn't work. Here's an example of a stack trace we're seeing:

/usr/local/google/home/nweiz/goog/dart/dart/out/ReleaseIA32/pub_async/lib/src/utils.dart 1:1412 syncFuture
/usr/local/google/home/nweiz/goog/dart/dart/out/ReleaseIA32/pub_async/lib/src/command.dart 1:3614 PubCommand.run
/usr/local/google/home/nweiz/goog/dart/dart/out/ReleaseIA32/pub_async/bin/pub.dart 7:1619 invokeCommand.<fn>
dart:async Future.Future.sync
/usr/local/google/home/nweiz/goog/dart/dart/out/ReleaseIA32/pub_async/lib/src/utils.dart 1:1417 syncFuture
/usr/local/google/home/nweiz/goog/dart/dart/out/ReleaseIA32/pub_async/bin/pub.dart 7:1596 invokeCommand
/usr/local/google/home/nweiz/goog/dart/dart/out/ReleaseIA32/pub_async/bin/pub.dart 1:1990 runPub.<fn>
dart:async Future.Future.sync
/usr/local/google/home/nweiz/goog/dart/dart/out/ReleaseIA32/pub_async/lib/src/utils.dart 1:1580 captureErrors.<fn>
package:stack_trace/src/chain.dart 93:20 Chain.capture
/usr/local/google/home/nweiz/goog/dart/dart/out/ReleaseIA32/pub_async/lib/src/utils.dart 1:1844 captureErrors
/usr/local/google/home/nweiz/goog/dart/dart/out/ReleaseIA32/pub_async/bin/pub.dart 1:1970 runPub
/usr/local/google/home/nweiz/goog/dart/dart/out/ReleaseIA32/pub_async/bin/pub.dart 1:1751 main.<fn>
dart:isolate _RawReceivePortImpl._handleMessage

"command.dart" is barely lower than the top level of pub. Without chain tracking, these async errors are effectively stack trace-free.

[dgrove]

This is really important for 1.7.

---

Added this to the 1.7 milestone.
Removed Priority-Unassigned label.
Added Priority-High label.

[lrhn]

I believe the async/await package doesn't actually handle stack-traces yet, so I wouldn't say that there is a problem with that yet.
I hope that we'll get reasonable stack-trace propagation with both the translation and with native async/await try/catch statements, so the exceptions and stack traces should be passed around predictably.

Synchronous code just throws normally, and has its correct stack trace.
That means that the synchronous parts of Future.sync will have a complete synchronous stack trace back to the last event/microtask entry. That stack trace is correct, but I guess you want to combine it with the stack-trace-trace that lead to that event, and don't have a hook to do so?

And the problem is the same for Completer.completeError - the stack trace might be just a synchronous stack trace, and you need to combine it with the list of stack traces that lead up to that event?

I hope we won't have to add another hook, even if it is only to modify the stack trace. Each hook costs, even when it's not used (because we have to check if it's used or call a nop-function). I can see that there current way to attach the improved stack trace to:
  new Future.sync(() { throw 42; }).catchError((e, s) { print(s); })

[nex3]

That stack trace is correct, but I guess you want to combine it with the stack-trace-trace that lead to that event, and don't have a hook to do so?

Right. We can do so most of the time, but because we lack the hook I described above there are cases we can't handle. We have a workaround we use right now—wrapping various calls and values in Chain.track—but that workaround doesn't work if the calls we need to track are automatically generated by async_await.

I hope we won't have to add another hook, even if it is only to modify the stack trace. Each hook costs, even when it's not used (because we have to check if it's used or call a nop-function).

I thought the point of Zones was that they imposed a minimal performance overhead when they weren't in use.

The hook I'm describing would only be used for exceptions anyway. Exceptions are very rarely used in performance-critical code.

[lrhn]

I thought the point of Zones was that they imposed a minimal performance overhead when they weren't in use.

Minimal, yes, but not zero, so the more places we have to check if the zones are in use, the more it adds up.

[lrhn]

Set owner to @lrhn.
Added Started label.

[lrhn]

I have committed an implementation of this.
It adds an "errorCallback" to Zone, and uses where an synchronous error or programmatic error enters an async context. Please try it out and see if it does enough (or too much).

[nex3]

Looks great! I have https://codereview.chromium.org//556363004 out to start using it in stack_trace. Thanks for working on this, it'll make a big difference in our async development experience.

[lrhn]

Added Fixed label.