nodejs / promises

Promises Working Group Repository
107 stars 11 forks source link

Alternative solutions for Post-mortem WG needs #24

Closed chrisdickinson closed 7 years ago

chrisdickinson commented 8 years ago

I open this question because there appears to be difficulty in "breaking out" of the promises error handling to provide a useful stack, but on the other hand the Chrome Dev Tools offer some abilities that mitigate this — specifically the ability to "step out" to asynchronous frames in their debugger. If we began to collect (or offered an option to collect) the necessary information to make this work for cores, while offering the ability to crash immediately on unhandled rejection, would this satisfy the needs of the PMWG? Outside of the Chrome Dev Tools, is there a way we can offer a tool, or modify the existing debuggers to be able to step back through frames in this fashion that would satisfy your needs?

/cc @nodejs/post-mortem

Any comments that refute the validity of stated needs of post-mortem WG members are off-topic and will be moderated off of this issue.

debug

misterdjules commented 8 years ago

I'm not sure I understand what this issue is about.

while offering the ability to crash immediately on unhandled rejection

If here "unhandled rejection" means "an error thrown synchronously that is not caught by a try/catch block or a promise's catch handler", and if "crash" here means "terminating the process and generating a core dump", then it seems that would be enough for post-mortem debugging users to be able to use a post-mortem debugging methodology using core files, and we wouldn't need to find an alternative.

So if what I described in the paragraph above is considered something we want to support as a prerequisite for landing a promises-based API in core, the rest of what is described in the first comment might be worth considering, but I would definitely not consider it as something that would be needed by post-mortem debugging users.

benjamingr commented 8 years ago

@misterdjules "Alternative solutions" - not to the post mortem group, to this PR.

misterdjules commented 8 years ago

@chrisdickinson

while offering the ability to crash immediately on unhandled rejection

If here "unhandled rejection" means "an error thrown synchronously that is not caught by a try/catch block or a promise's catch handler",

I realize that I misinterpreted your comment, and that by "on unhandled rejection" you meant "when the callback set by V8's SetPromiseRejectCallback API is called". My apologies, I'll try to post a detailed response tomorrow (Thursday) :)

In the meantime, and in order to prevent me from looking at the wrong thing, do you have some pointers (other than the inline screenshot) on what you refer to with "Chrome Dev Tools offer some abilities that mitigate this — specifically the ability to "step out" to asynchronous frames in their debugger."?

benjamingr commented 8 years ago

As an alternative solution - if we scope all the promised core under promised/* then the post-mortem using users who don't like the error handling model can keep blacklisting things. That would mean promises would not interfere with the needs of that group.

chrisdickinson commented 8 years ago

@misterdjules:

In the meantime, and in order to prevent me from looking at the wrong thing, do you have some pointers (other than the inline screenshot) on what you refer to with "Chrome Dev Tools offer some abilities that mitigate this — specifically the ability to "step out" to asynchronous frames in their debugger."?

Unfortunately I'm not familiar with how they've implemented their async stack frame collection. I'll take a look into it tomorrow to see if I can give a summary of what they're doing here.

@benjamingr:

As an alternative solution - if we scope all the promised core under promised/* then the post-mortem using users who don't like the error handling model can keep blacklisting things. That would mean promises would not interfere with the needs of that group.

The require('promised/*') will not necessarily hide all promise APIs — blacklisting those modules probably won't be sufficient. The global.Promise = null preload module approach will work, though.

ofrobots commented 8 years ago

Unfortunately I'm not familiar with how they've implemented their async stack frame collection

My understanding is that this works very similarly to how long stack-traces work. Basically, you capture a context (stack trace) on the before side of the async API and stitch it back from on the after side. There is no magic. This can be expensive.

bmeck commented 8 years ago

@chrisdickinson for the Chromium (not v8) async stack traces these are the main points of interest:

Actual stack recording V8AsyncCallTracker::didReceiveV8AsyncTaskEvent

Actual stack trace presentation V8DebuggerAgentImpl::currentAsyncStackTraceForRuntime

benjamingr commented 8 years ago

@bmeck correct me if I'm wrong, but that looks like iteration through JavaScriptCallFrame which has access to context like the this value.

(Which means we can, if we manage to hook on this mechanism address a big part of (but not all of) what the post-mortem group would like to have without crashing the process)

bmeck commented 8 years ago

@benjamingr yes though the values may mutate and be incorrect unless we fully record on each enqueue

benjamingr commented 8 years ago

@bmeck can you please show me (and document) a practical use case where that's critical and actual mutation caused problems for debugging? I understand that theoretically this can happen but I'm wondering if it does.

petkaantonov commented 8 years ago

The values on the recorded stack do not change but if the values are references to mutable objects, the objects referenced may theoretically be destructively mutated. For typical code that doesn't reference application wide global mutable objects but rather either pooled objects (which are presumably not returned back to the pool if the code failed in irrecoverable ways) or objects that are only seen by the failed context I think this concern is more theoretical than pragmatic. I could be wrong though.

benjamingr commented 8 years ago

@petkaantonov yes that's my impression as well - but I'm interested in knowing if there are any realistic use cases where this distinction is meaningful.

bmeck commented 8 years ago

@benjamingr real world situations are eluding me but this is pretty easy to see via a debugger.

function f(a,b) {
  let c = new Promise((f,r) => {throw new Error});
  c.then(_ => {debugger;console.log(c);}) // open up a debugger, a/b are not in scope/can be GC'd by this point
  return c;
}
f(123,456);
bmeck commented 8 years ago

most of the time the value in post mortem debugging is you can look for the information to figure out the problem, doing the inverse is hard as most bugs are per case.

benjamingr commented 7 years ago

Going to progress with util.awaitable since #5020 is pretty dead.