Add a stack chain class to the stack trace package.

Add a stack chain class to the stack trace package.

This class uses zones to track stack traces across asynchronous boundaries. It
will aid considerably in debugging errors in heavily-asynchronous programs.

R=floitsch@google.com, rnystrom@google.com
BUG=7040
Committed: https://code.google.com/p/dart/source/detail?r=30738

[nweiz, 2013-11-20 20:17:20]

Adding Florian at Bob's request to make sure this is interacting with the Zone
API in a sensible way.

[Bob Nystrom, 2013-11-20 20:19:52]

https://codereview.chromium.org/75863004/diff/1/pkg/stack_trace/lib/src/chain...
File pkg/stack_trace/lib/src/chain.dart (right):

https://codereview.chromium.org/75863004/diff/1/pkg/stack_trace/lib/src/chain...
pkg/stack_trace/lib/src/chain.dart:23: /// reset as each asynchronous callback
is run.
"reset" -> "unwound", "run" -> "registered".

https://codereview.chromium.org/75863004/diff/1/pkg/stack_trace/lib/src/chain...
pkg/stack_trace/lib/src/chain.dart:74: /// issues 15171 and 15105. Stack chains
reported on dart2js will contain only
Since this is in a doc comment, link to these bugs.

https://codereview.chromium.org/75863004/diff/1/pkg/stack_trace/lib/src/stack...
File pkg/stack_trace/lib/src/stack_zone_specification.dart (right):

https://codereview.chromium.org/75863004/diff/1/pkg/stack_trace/lib/src/stack...
pkg/stack_trace/lib/src/stack_zone_specification.dart:49: /// The front-most
node of the current stack chain.
"front-most" is a bit ambiguous. "most recent"?

https://codereview.chromium.org/75863004/diff/1/pkg/stack_trace/lib/src/stack...
pkg/stack_trace/lib/src/stack_zone_specification.dart:53: : super();
Is this needed?

https://codereview.chromium.org/75863004/diff/1/pkg/stack_trace/lib/src/stack...
pkg/stack_trace/lib/src/stack_zone_specification.dart:198: var queue = new
Queue<Trace>();
Why not just a list?

https://codereview.chromium.org/75863004/diff/1/pkg/stack_trace/test/chain_te...
File pkg/stack_trace/test/chain_test.dart (right):

https://codereview.chromium.org/75863004/diff/1/pkg/stack_trace/test/chain_te...
pkg/stack_trace/test/chain_test.dart:73: // O(depth of program), not O(length of
progam).
I want to understand this better. Can you walk me through it tomorrow?

https://codereview.chromium.org/75863004/diff/40001/pkg/stack_trace/test/chai...
File pkg/stack_trace/test/chain_test.dart (right):

https://codereview.chromium.org/75863004/diff/40001/pkg/stack_trace/test/chai...
pkg/stack_trace/test/chain_test.dart:38:
contains(frameMember(startsWith('inOneShotTimer'))));
How about a helper function for these expectations, so you can do:

expectTraces(chain, [
  'inOneShotTimer'
]);

And it will handle the boilerplate of these tests?

[nweiz, 2013-11-20 21:30:12]

https://codereview.chromium.org/75863004/diff/1/pkg/stack_trace/lib/src/chain...
File pkg/stack_trace/lib/src/chain.dart (right):

https://codereview.chromium.org/75863004/diff/1/pkg/stack_trace/lib/src/chain...
pkg/stack_trace/lib/src/chain.dart:23: /// reset as each asynchronous callback
is run.
On 2013/11/20 20:19:53, Bob Nystrom wrote:
> "reset" -> "unwound", "run" -> "registered".

I think my wording is correct here. Registering a callback doesn't affect the
stack; it's just storing it in some variable. When the callback is run, though,
the stack is reset entirely, rather than just unwinding to a higher point.

https://codereview.chromium.org/75863004/diff/1/pkg/stack_trace/lib/src/chain...
pkg/stack_trace/lib/src/chain.dart:74: /// issues 15171 and 15105. Stack chains
reported on dart2js will contain only
On 2013/11/20 20:19:53, Bob Nystrom wrote:
> Since this is in a doc comment, link to these bugs.

Done.

https://codereview.chromium.org/75863004/diff/1/pkg/stack_trace/lib/src/stack...
File pkg/stack_trace/lib/src/stack_zone_specification.dart (right):

https://codereview.chromium.org/75863004/diff/1/pkg/stack_trace/lib/src/stack...
pkg/stack_trace/lib/src/stack_zone_specification.dart:49: /// The front-most
node of the current stack chain.
On 2013/11/20 20:19:53, Bob Nystrom wrote:
> "front-most" is a bit ambiguous. "most recent"?

Done.

https://codereview.chromium.org/75863004/diff/1/pkg/stack_trace/lib/src/stack...
pkg/stack_trace/lib/src/stack_zone_specification.dart:53: : super();
On 2013/11/20 20:19:53, Bob Nystrom wrote:
> Is this needed?

No, this is a leftover from when I was trying to extend ZoneSpecification.

https://codereview.chromium.org/75863004/diff/1/pkg/stack_trace/lib/src/stack...
pkg/stack_trace/lib/src/stack_zone_specification.dart:198: var queue = new
Queue<Trace>();
On 2013/11/20 20:19:53, Bob Nystrom wrote:
> Why not just a list?

I used to be using addFirst here, but then I switched it around. Changed to
List.

https://codereview.chromium.org/75863004/diff/1/pkg/stack_trace/test/chain_te...
File pkg/stack_trace/test/chain_test.dart (right):

https://codereview.chromium.org/75863004/diff/1/pkg/stack_trace/test/chain_te...
pkg/stack_trace/test/chain_test.dart:73: // O(depth of program), not O(length of
progam).
On 2013/11/20 20:19:53, Bob Nystrom wrote:
> I want to understand this better. Can you walk me through it tomorrow?

Done.

[Bob Nystrom, 2013-11-20 22:37:04]

I don't feel like I have a deep grasp of what this does, but the code LGTM.

[floitsch, 2013-11-21 15:28:08]

I'm impressed. This looks great.

Lots of minor nits (you can ignore most of them) and a few comments, but LGTM!

https://codereview.chromium.org/75863004/diff/120001/pkg/stack_trace/lib/src/...
File pkg/stack_trace/lib/src/chain.dart (right):

https://codereview.chromium.org/75863004/diff/120001/pkg/stack_trace/lib/src/...
pkg/stack_trace/lib/src/chain.dart:23: /// reset as each asynchronous callback
is run.
"as" feels wrong to me.

https://codereview.chromium.org/75863004/diff/120001/pkg/stack_trace/lib/src/...
pkg/stack_trace/lib/src/chain.dart:52:
Zone.current[#stack_trace.stack_zone.spec];
missing indent.

https://codereview.chromium.org/75863004/diff/120001/pkg/stack_trace/lib/src/...
pkg/stack_trace/lib/src/chain.dart:74: /// issues [15171] and [15105]. Stack
chains reported on dart2js will contain only
long line.

https://codereview.chromium.org/75863004/diff/120001/pkg/stack_trace/lib/src/...
File pkg/stack_trace/lib/src/stack_zone_specification.dart (right):

https://codereview.chromium.org/75863004/diff/120001/pkg/stack_trace/lib/src/...
pkg/stack_trace/lib/src/stack_zone_specification.dart:14: /// Until they're
materialized and exposed to the user, stack chains are tracked
nit: they are

https://codereview.chromium.org/75863004/diff/120001/pkg/stack_trace/lib/src/...
pkg/stack_trace/lib/src/stack_zone_specification.dart:56: handleUncaughtError:
handleUncaughtError,
If _onError is null, this should probably be set to null.

This is not just an efficiency question: Futures and Streams do not cross
error-zone boundaries (where an "error-zone" is a zone that has an
handleUncaughtError handler).

This is a restriction that wasn't planned. We will discuss if we can find ways
to intercept uncaught errors without being an error-zone.

https://codereview.chromium.org/75863004/diff/120001/pkg/stack_trace/lib/src/...
pkg/stack_trace/lib/src/stack_zone_specification.dart:69: /// Returns the stack
chain associated with, [trace], if one exists.
spurious "," ?

https://codereview.chromium.org/75863004/diff/120001/pkg/stack_trace/lib/src/...
pkg/stack_trace/lib/src/stack_zone_specification.dart:86: var completer = new
Completer();
This is the perfect use case for a sync-completer.

https://codereview.chromium.org/75863004/diff/120001/pkg/stack_trace/lib/src/...
pkg/stack_trace/lib/src/stack_zone_specification.dart:88:
future.then(completer.complete).catchError((e, stackTrace) {
might as well use future.then(completer.complete, onError: (e, stackTrace) ...)

https://codereview.chromium.org/75863004/diff/120001/pkg/stack_trace/lib/src/...
pkg/stack_trace/lib/src/stack_zone_specification.dart:93: return
completer.future;
there is a corner-case:
future.catchError(...);
var f2 = trackFuture(future);
f2.then(...)

if the future completes with an error, the future returned by trackFuture will
not have an error-listener and yield an unhandled error.

I would say this is not important enough, though. Just wanted to mention it.

https://codereview.chromium.org/75863004/diff/120001/pkg/stack_trace/lib/src/...
pkg/stack_trace/lib/src/stack_zone_specification.dart:115: Function f) {
ZoneCallback

https://codereview.chromium.org/75863004/diff/120001/pkg/stack_trace/lib/src/...
pkg/stack_trace/lib/src/stack_zone_specification.dart:124: Zone zone, Function
f) {
ZoneUnaryCallback

https://codereview.chromium.org/75863004/diff/120001/pkg/stack_trace/lib/src/...
pkg/stack_trace/lib/src/stack_zone_specification.dart:135: Zone zone, Function
f) {
ZoneBinaryCallback

https://codereview.chromium.org/75863004/diff/120001/pkg/stack_trace/lib/src/...
pkg/stack_trace/lib/src/stack_zone_specification.dart:148: return
parent.handleUncaughtError(zone, error, chainFor(stackTrace));
This looks wrong.
handleUncaughtError requires the last argument to be a StackTrace.

https://codereview.chromium.org/75863004/diff/120001/pkg/stack_trace/test/cha...
File pkg/stack_trace/test/chain_test.dart (right):

https://codereview.chromium.org/75863004/diff/120001/pkg/stack_trace/test/cha...
pkg/stack_trace/test/chain_test.dart:73: // O(depth of program), not O(length of
progam).
program

https://codereview.chromium.org/75863004/diff/120001/pkg/stack_trace/test/cha...
pkg/stack_trace/test/chain_test.dart:607: Future<Chain> capture(callback()) {
Please rename, or at least put at the top.
It took me several minutes before I realized that "capture" is not the
Chain.capture.

[nweiz, 2013-11-22 00:50:52]

https://codereview.chromium.org/75863004/diff/120001/pkg/stack_trace/lib/src/...
File pkg/stack_trace/lib/src/chain.dart (right):

https://codereview.chromium.org/75863004/diff/120001/pkg/stack_trace/lib/src/...
pkg/stack_trace/lib/src/chain.dart:23: /// reset as each asynchronous callback
is run.
On 2013/11/21 15:28:08, floitsch wrote:
> "as" feels wrong to me.

Changed to "before".

https://codereview.chromium.org/75863004/diff/120001/pkg/stack_trace/lib/src/...
pkg/stack_trace/lib/src/chain.dart:52:
Zone.current[#stack_trace.stack_zone.spec];
On 2013/11/21 15:28:08, floitsch wrote:
> missing indent.

I like using two spaces here because this is basically the body of a function. I
think of "=>" as a drop-in replacement for "{ return ... }".

https://codereview.chromium.org/75863004/diff/120001/pkg/stack_trace/lib/src/...
pkg/stack_trace/lib/src/chain.dart:74: /// issues [15171] and [15105]. Stack
chains reported on dart2js will contain only
On 2013/11/21 15:28:08, floitsch wrote:
> long line.

Done.

https://codereview.chromium.org/75863004/diff/120001/pkg/stack_trace/lib/src/...
File pkg/stack_trace/lib/src/stack_zone_specification.dart (right):

https://codereview.chromium.org/75863004/diff/120001/pkg/stack_trace/lib/src/...
pkg/stack_trace/lib/src/stack_zone_specification.dart:56: handleUncaughtError:
handleUncaughtError,
On 2013/11/21 15:28:08, floitsch wrote:
> If _onError is null, this should probably be set to null.
> 
> This is not just an efficiency question: Futures and Streams do not cross
> error-zone boundaries (where an "error-zone" is a zone that has an
> handleUncaughtError handler).
> 
> This is a restriction that wasn't planned. We will discuss if we can find ways
> to intercept uncaught errors without being an error-zone.

Even if the user isn't handling the errors, we want to translate the stack
traces to chains. This does mean that it's always an error zone, which may
confuse users, but I think it's a worthwhile tradeoff for making it possible to
just throw [Chain.capture] at the top level of the program and have something
good happen.

I'll add a comment about this to the [Chain.capture] docs.

https://codereview.chromium.org/75863004/diff/120001/pkg/stack_trace/lib/src/...
pkg/stack_trace/lib/src/stack_zone_specification.dart:69: /// Returns the stack
chain associated with, [trace], if one exists.
On 2013/11/21 15:28:08, floitsch wrote:
> spurious "," ?

Done.

https://codereview.chromium.org/75863004/diff/120001/pkg/stack_trace/lib/src/...
pkg/stack_trace/lib/src/stack_zone_specification.dart:86: var completer = new
Completer();
On 2013/11/21 15:28:08, floitsch wrote:
> This is the perfect use case for a sync-completer.

Done.

https://codereview.chromium.org/75863004/diff/120001/pkg/stack_trace/lib/src/...
pkg/stack_trace/lib/src/stack_zone_specification.dart:88:
future.then(completer.complete).catchError((e, stackTrace) {
On 2013/11/21 15:28:08, floitsch wrote:
> might as well use future.then(completer.complete, onError: (e, stackTrace)
...)

I prefer using [catchError] unless I want to explicitly handle the case where
the success callback throws an error that shouldn't be passed to the [onError]
callback.

https://codereview.chromium.org/75863004/diff/120001/pkg/stack_trace/lib/src/...
pkg/stack_trace/lib/src/stack_zone_specification.dart:115: Function f) {
On 2013/11/21 15:28:08, floitsch wrote:
> ZoneCallback

According to
http://test.dartdoc-next.appspot.com/#dart-async.RegisterCallbackHandler, this
is typed as a Function. I'd like to keep this in line with the docs, even if the
function will always be a ZoneCallback.

Same goes for unary and binary handlers.

https://codereview.chromium.org/75863004/diff/120001/pkg/stack_trace/lib/src/...
pkg/stack_trace/lib/src/stack_zone_specification.dart:148: return
parent.handleUncaughtError(zone, error, chainFor(stackTrace));
On 2013/11/21 15:28:08, floitsch wrote:
> This looks wrong.
> handleUncaughtError requires the last argument to be a StackTrace.

Chain implements StackTrace.

https://codereview.chromium.org/75863004/diff/120001/pkg/stack_trace/test/cha...
File pkg/stack_trace/test/chain_test.dart (right):

https://codereview.chromium.org/75863004/diff/120001/pkg/stack_trace/test/cha...
pkg/stack_trace/test/chain_test.dart:73: // O(depth of program), not O(length of
progam).
On 2013/11/21 15:28:08, floitsch wrote:
> program

Done.

https://codereview.chromium.org/75863004/diff/120001/pkg/stack_trace/test/cha...
pkg/stack_trace/test/chain_test.dart:607: Future<Chain> capture(callback()) {
On 2013/11/21 15:28:08, floitsch wrote:
> Please rename, or at least put at the top.
> It took me several minutes before I realized that "capture" is not the
> Chain.capture.

Done.

[nweiz, 2013-11-27 23:03:58]

Committed patchset #4 manually as r30738 (presubmit successful).