Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stack / call history support for async sequences of promises? #108

Closed
gr2m opened this issue Feb 16, 2013 · 18 comments
Closed

stack / call history support for async sequences of promises? #108

gr2m opened this issue Feb 16, 2013 · 18 comments

Comments

@gr2m
Copy link

gr2m commented Feb 16, 2013

Let's say I've this sequence of promise, all running async returning new promises again

this.doThat()
.then ( this.doThatToo )
.then ( this.andDoThis )
.then ( this.butFailHere )
.otherwise ( this.handleError )

Let's say that this.butFailHere will throw an error that gets passed to this.handleError. Is there any way I could show a stack of functions that have been called until the error occured?

Desired output would be something like this

Error: Chuck Norris told me to fail
    at Worker.butFailHere (path/to/worker.js:400)
    at Worker.andDoThis (path/to/worker.js:300)
    at Worker.doThatToo (path/to/worker.js:200)
    at Worker.doThat (path/to/worker.js:100)
@briancavalier
Copy link
Member

Hey @gr2m, this is an excellent question, and there's quite a bit wrapped up in it :)

The root of the problem is that asynchronous operations cross stack boundaries, i.e. the stack is most likely cleared between each of doThat, doThatToo, andDoThis, butFailHere, and handleError.

We've talked about ways to track stack jumps, and it is doable. Basically, when.js would have to generate stack traces internally at several key points, filter out when.js-specific lines, and record the result such that they could be stitched back together later into something relatively coherent and meaningful.

This is something we could support in when/debug since it could be a significant performance hit. I don't think we'd want to support it in non-debug mode.

To be honest, It hasn't really been a priority for us yet, but I do appreciate that promises can be difficult to debug, and I would love to find ways to help! I wonder if @Riccieri or @Twisol would be interested in taking this on as a project?

@briancavalier
Copy link
Member

Error: Chuck Norris told me to fail

Oh, and ... LOL

@gr2m
Copy link
Author

gr2m commented Feb 16, 2013

Thanks for the response! Would be great to have an option for that in when/debug.

I wonder if that's actually a common use case? Are there any typical workarounds that I could use until it's build into when.js?

@renato-zannon
Copy link
Contributor

That is something I'm interested on, I've faced a few confusing stack traces myself :) I could research that!

I know that there're a few existing projects for making stack traces on JS. Taking an optional dependency is an option for when/debug?

@briancavalier
Copy link
Member

Awesome. Thanks @Riccieri!

Yeah, we can pretty much do anything we need with when/debug, so adding a dependency that helps us implement this would be ok. My preference would be to implement it all ourselves, but if that's prohibitive, we can pull in 3rd party code. We'd need to make sure it supports all the environments we care about, and decide the best mechanism for pulling it in (npm, bower, commit to when.js repo, copy/paste, etc.).

The stack trace collection and stitching mechanisms will be the challenging part, for sure.

@Twisol
Copy link
Contributor

Twisol commented Feb 16, 2013

I think the biggest potential issue is that each browser formats its stack traces differently. I've only seen this successfully done in Node.js, where you're always using V8, or in libraries that make engine-specific assumptions.

Since there's no standard on stack traces to aim for, the best we could do is a whitelisting approach: feature-detect on stack traces and include an implementation for each variant. I dislike this, but since it's for when/debug it may not be so horrible.

@briancavalier
Copy link
Member

@gr2m Alas, I'm not aware of any existing 3rd party tools for easily collecting and stitching disconnected stack frames in JS. If you do manage to find something, please let us know--it may be useful in implementing this feature!

I just created #109 to track this new feature in when/debug.

@briancavalier
Copy link
Member

@Twisol Hmmm, yes, good points. Also, Chrome (or maybe this is a v8 thing) has some new APIs for dealing with stack traces that we should investigate. If the first version of this feature only worked in v8, I would probably be ok with that since this would be for debugging only. We could look into ways to support other environments as a follow-on exercise.

@gr2m
Copy link
Author

gr2m commented Feb 16, 2013

@briancavalier will do! Thanks for the support!

The only naive approach I can think of is to manually do something like .otherwise( addToErrorStack('doThis()') ) after every promise function I want to track. I feel there must be more elegant solutions for that

@gr2m
Copy link
Author

gr2m commented Feb 16, 2013

I'd also not aim to make this promise stack thing look like the original stack of the respective browser. Because the alternative is no stack at all. Even if it would be a very reduced array of function names, it would be a hupe help already

@Twisol
Copy link
Contributor

Twisol commented Feb 16, 2013

Right - I think the trace should be split into two parts: the native trace we already have, which shows your standard line of execution, and an augmentation that shows the handlers that have been executed earlier in the promise chain (probably stopping at the point where the promise was resolved?)

Example:

function a() { b(); }
function b() { c(); }
function c() { throw new Error("Chuck Norris told me to fail"); }

this.doThat().resolve(42)
  .then(this.doThatToo)
  .then(this.andDoThis)
  .then(function butFailHere() {
    a();
  })
  .otherwise(this.handleError);


  Error: Chuck Norris told me to fail
      at c (path/to/worker.js:502)
      at b (path/to/worker.js:501)
      at a (path/to/worker.js:500)
      at butFailHere (path/to/worker.js:400)
  ------------------------------------------------------------
      after Worker.andDoThis (path/to/worker.js:300)
      after Worker.doThatToo (path/to/worker.js:200)
      after Worker.doThat (path/to/worker.js:100)
      after whereItWasResolved (path/to/worker.js:396)

@briancavalier
Copy link
Member

I'd also not aim to make this promise stack thing look like the original stack of the respective browser

Agree, a synchronous-stack-trace-look-alike probably will not be as helpful as something tailored toward helping to debug disjoint stacks. Seems axiomatically true, in fact :) Something like what @Twisol has proposed seems like a good direction.

I'll add that we'd also need to track explicit rejections, e.g. deferred.reject and deferred.resolver.reject, so we can output that in cases where an explicit rejection, rather than an implicit rejection (thrown exception, etc.) is the root cause.

Right now, I envision this working like current VMs work for exceptions: they are only loud about unhandled exceptions that reach the top of the call stack. So, perhaps we should only be loud about rejections that "escape" the end of a promise chain. We could record information about all rejections, but by default, I think logging all of them might be at least annoying and probably overwhelming (imagine if your browser JS console logged every thrown exception!).

This is tricky because it's basically the halting problem for promises. At any point in time t you can't say that a rejected promise will never be handled, since at time t+1 someone might add a rejection handler to it, and handle the rejection by returning successfully, thus turning it back into a fulfillment which shouldn't have been logged.

@Twisol
Copy link
Contributor

Twisol commented Feb 19, 2013

Once you've resolved the promise, and the chain is executing, 'then' should not add to the same chain. So you have a guarantee at time of execution: when a rejection reaches a dead end, no handler will ever be added to that chain in the same so-called timeline.

It would be silly, IMO, to hold up an error on the off chance that someone may one day come back and, in an new timeline, handle the error. They should have chained onto it earlier if they expected the possibility of an error.

@briancavalier
Copy link
Member

Unfortunately, people make lots of mistakes with promises. We should strive to help them. If we assume they always do it right and never get confused, then this feature is less important.

Adding a rejection handler after a promise has resolved/rejected is a valid use case. In a highly async system where promises are passed around freely, consumed from and given to 3rd party libraries, it's not really valid to assume that all handlers will always be added before a promise resolves or rejects.

@Twisol
Copy link
Contributor

Twisol commented Feb 19, 2013

Yes, agreed - but the new handler must execute in a new "timeline" to preserve the semantics of 'then'. If a decision must be made as to when to throw an error, then it should be either at the end of the current timeline, or never.

@briancavalier
Copy link
Member

Thanks for clarifying. "Never" is what we do now, LOL :) After a timeout could be acceptable as well. Yes, that's arbitrary, but it's for debugging so I'm in favor of whichever is most helpful for the developer.

I can imagine an approach where the current set of "currently known" unhandled rejections is maintained by adding and removing from it as promises are rejected, then later handled, etc etc. A periodic task could check that list for certain criteria, and then print diagnostic information for each unhandled rejection that matches.

@briancavalier
Copy link
Member

We should move any further discussion of possible approaches over to #109

@briancavalier
Copy link
Member

Closing in favor of #109

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants