promises and post-mortem debugging

[misterdjules]

A PR that aims at adding a promises-based API to most node core APIs was recently submitted to nodejs/node.

I have expressed my concerns regarding the impact of such a change on post-mortem debugging, and a separate issue was created by @Raynos to further discuss these concerns.

My concerns are more questions that I asked because I felt that the way promises handle errors is incompatible, at least in some ways, with post-mortem debugging. So far my investigations only confirmed these concerns, but I'm not done with them. One part of these investigations is also to find a solution to the problems that I found so far.

The discussion in nodejs/node has been productive so far, but I represent only one post-mortem debugging user/maintainer. I feel it would help if some people from this working group would share their opinion on these concerns with the rest of the group here in this issue, if only to confirm or infirm that they have the same questions/reserves as I do about the impact of https://github.com/nodejs/node/pull/5020 on post-mortem debugging. For instance, if some of you already worked on that problem and did a similar investigation, I would be interested if you could share your results.

In any case, I will share my findings with the group as soon as I have some more time to continue my investigations.

Thank you!

[bmeck]

@benjamingr v8 currently only does basic processing of the MicrotaskQueue (Promise JobQueue) through RunMicrotasks and EnqueueMicrotask. We cannot inspect the queue at all.

• We don't know when things are put on it (something like EnqueueMicrotaskCallback).
• Errors are completely swallowed so we don't get the real stack trace from rejections. If we could enforce all tasks to be wrapped via another functions this could be fixed, though not possible with v8 currently.
• We can't stop executing the MicrotaskQueue, once we hit an error, we just keep executing (very YOLO).
• The task queue cannot be filtered (slightly off topic for this discussion). We cannot selectively execute certain tasks. Once again we could wrap and requeue if we had the instrumentation to do so, but that is terrible.

[benjamingr]

That's a good summary. For what it's worth, that is all exposed for instrumentation in bluebird (via setScheduler and its friends).

Errors are completely swallowed so we don't get the real stack trace from rejections.

Can I see an example of this?

We can't stop executing the MicrotaskQueue, once we hit an error, we just keep executing

If errors are swallowed, how does hitting an error look like? Is the wrapping performed at the promise level or at a higher abstraction level?

Is what's missing hooks for scheduling?

I think it would be a good idea to open a separate thread to discuss all issues related to how Node schedules v8 promises and what we'd like to improve for better instrumentation for debugging.

[bmeck]

@benjamingr Node does not schedule Promises, v8 does.

Can I see an example of this?

Cause a non-Error to be thrown.

Promise.resolve(1)
  .then(() => {
    throw 1;
  })
  .catch(console.error)

If errors are swallowed, how does hitting an error look like?

If the task is a Promise, it rejects without any notification to observe synchronously; otherwise... silence.

[benjamingr]

it rejects without any notification to observe synchronously

Pretty sure there is a hook for that which we used in `unhandledRejection"

Cause a non-Error to be thrown.

Yes, that's awful, but it's mostly similar in stack traces in synchronous code isn't it? A non error doesn't have a stack property. Bluebird used to create an error in the background for stack traces even for throw 1 but we dropped it since everyone just throws errors exclusively - we warn on non-errors though.

[bmeck]

@benjamingr

process.on('unhandledRejection', ()=>{console.log('unhandled rejection')});
const p = Promise.resolve(1)
p.then(_ => {throw 1;})
p.then(_ => console.log('not-sync'))

[vkurchatkin]

Node does not schedule Promises, v8 does.

node does schedule microtask queue runs, though

We can't stop executing the MicrotaskQueue, once we hit an error, we just keep executing

This is find, cause microtask queue is used only for promise handlers, which don't throw errors

[benjamingr]

@bmeck yes, unhandledRejection is not tracked synchronously by design, it is tracked after the microtask queue has been flushed. What is synchronous is the handler V8 exposes:

isolate->SetPromiseRejectCallback(PromiseRejectCallback);

Which we use here: https://github.com/nodejs/node/blob/master/src/node.cc#L1121

[bmeck]

@vkurchatkin

• any mutations further down the queue will be present

process.on('unhandledRejection', ()=>{process.exit(1);});
const p = Promise.resolve(1)
// try to blow up process
p.then(_ => {throw 1;})
p.then(_ => process.removeAllListeners('unhandledRejection')); // stupid but very visible example

• if the handler queues other things doesn't it add it to the end rather than letting us check if there was an unhandled rejection

process.on('unhandledRejection', ()=>{process.exit(1);});
const p = Promise.resolve(1)
// gonna blow up the proc
p.then(_ => {throw 1;})
function forever() {return Promise.resolve(0).then(forever);}
forever();

[benjamingr]

@bmeck

any mutations further down the queue will be present

"unhandledRejection" has a test that asserts this behavior. You're supposed to be able to react to changes for as long as you want while you're in the microtick queue.

if the handler queues other things doesn't it add it to the end rather than letting us check if there was an unhandled rejection

Unhandled rejections are explicitly run after all the microtasks have finished executing. This is again by design in order to stop bad behavior - the discussion on why it works this way is at https://github.com/nodejs/node/pull/758

Of course it we want to illustrate things, here is the similar example synchronously:

process.nextTick(() => process.exit(1)); 
while(1);

(I think it would be really cool to move this discussion to another thread btw).

[bmeck]

@benjamingr not stating sync doesn't have issues, just stating the problems currently w/ post-mortem and promises workflows.

[benjamingr]

@bmeck I'm not complaining :) I'm just giving an analog. I'm learning stuff.

[bmeck]

@benjamingr basically to get the same level of information as we currently get from uncaughtException causing abort we would need SetPromiseRejectCallback to generate a core file every time it fires. This is not realistic. Anything after that point will:

1. lose the stack/context
   • lose the arguments
   • lose the closure
2. mutate application state
   • lose the state of the program when we had problems
   • potentially GC relevant data

A lot of these are not able to be solved purely in JS, and the mutation bit basically makes post mortem crippled / a poor choice since your data could be wrong.

[misterdjules]

@bmeck

basically to get the same level of information as we currently get from uncaughtException causing abort we would need SetPromiseRejectCallback to generate a core file every time it fires

It doesn't seem that even works as we'd need it to work, as described in one experiment I made earlier.

I made another experiment that solves some of the use cases, but that still has fundamental issues.

Please note that the document I'm linking to is a draft.

[benjamingr]

@bmeck thanks, see my proposal https://github.com/groundwater/nodejs-symposiums/pull/5#issuecomment-182822340

[spion]

@misterdjules excellent document. Indeed promises are fundamentally incompatible with post-mortem debugging.

Even if we were somehow able to get extended captureStackTrace functionality, unless we clone the entire chunk of the process state that we're interested in there will be no guarantee that by the time an unhandledRejection event happens (must be at least after the call stack fully unwinds due to how catch handlers are attached to promises) the data will look the same. Additionally, cloning on every generated error is prohibitively expensive when using promises, even if only done when a thrown error is caught: I believe most promise users liberally use throw within callbacks passed to then (I definitely do).

function promiseReturningOperation(arg) {
  if (iDontLike(arg)) throw new TypeError("Your argument is invalid!")
  else return proceedWithSomeAsyncChain()
}
function f(arg1, arg2) {
  var p = promiseReturningOperation(arg1) // throws
  arg1.foo = "bar"; // we must let this execute, a catch handler may be added to returned `p`
  return p
}

f("test", "test").catch(e => console.error(e))

The best that can be done is adding a 1-tick delayed picture of the function arguments in the call stack, and dump core to get a delayed picture of the rest of the process state.

[benjamingr]

@spion I think we can generate a good heap dump if catch handlers are added before the then handlers execute which is almost always the case but is less relaxed than unhandledRejection. It does so only on the first unhandled rejection so errors can be fixed while the server is running and keep it in consistent state.

https://github.com/groundwater/nodejs-symposiums/pull/5#issuecomment-182822340

[spion]

@benjamingr I added a code sample above that demonstrates we cannot do it. It shows that the global state or the state of the function arguments can be altered between the point where the error happens and the point where "unhandledRejection" has been triggered.

Unlike with regular exception propagation, which does not allow additional code execution except for matching catch blocks, promise exception propagation must allow it. Thats because attaching the catch handler is itself an additional execution "step", one that isn't special and cannot be differentiated from other steps (like changing state).

[benjamingr]

@spion that proposal does not rely on "unhandledRejection", it dumps core if a .catch handler has not been already attached to a promise.

getHttpResourceFn().then(v => {
    const foo = doSomethingWith(v); // if this throws, we enter the catch handler
    return foo;   
}).catch(e => {
    console.log("Oh No");
});

However if the catch handler is not attached to the promise before the then executes - it will heap-dump with the flag.

[spion]

@benjamingr Try to rewrite the sample code above to conform to that.

[spion]

@benjamingr Nevermind, as you pointed out, the example has problems (catch wont work because the error is synchronously thrown)

So that solution relies on delaying the execution of code that may throw a sync exception until after catch handlers have been added. Interesting. I'll let you know if I manage to break that assumption.

[benjamingr]

@spion note that no changes are required - this is already the case since then handlers and catch handlers never execute synchronously so catch has already been added by the time the error is thrown.

This is only problematic in the promise constructor in wild uses in userland code and only when it throws synchronously.

This means we can promisify core and get good post-mortem debugging but we still don't have a solution for the promise constructor. I think that's the case today anyway though.

Maybe --abort-on-sync-unhandled-rejection-once should core-dump if an error is thrown in the promise constructor.

[misterdjules]

@spion

Indeed promises are fundamentally incompatible with post-mortem debugging.

I'm not sure we can say that before having explored every potential solutions we can think of and convincing ourselves that they don't work. The conclusion in my document explicitly says that it's unlikely that the currently perceived incompatibility can be solved with changes to the implementation, but I'm still trying to prove me wrong :)

Have you read the second potential solution described in that document? It does not rely on the unhandledRejection event, allows the generation of a core file at the exact time an uncaught exception was thrown, and thus preserves the state of process in that core file at that time. It seems that it might be a step towards the right direction.

[benjamingr]

Please see my comment a few comment above @misterdjules I think it's similar to your idea only it works if the rejection is caught.

[benjamingr]

I discussed this with spion on Facebook earlier today and he also reached the same conclusion.

It solves the post mortem debugging issue but makes an opinionated choice on promise usage. It alters the behavior based on the flag but only a little.

[misterdjules]

@benjamingr

Please see my comment a few comment above

Are you refering to what you described in the nodejs-symposiums repository?

I think it's similar to your idea only it works if the rejection is caught.

Do you mean that the second potential solution I described in my document does make the process abort and does not display 'caught' in the following use case:

function foo() {
  throw new Error('boom');
}

function boom() {
  foo();
}
var boomPromise = new Promise(function (resolve, reject) {
  boom();
}).catch(function() {
  console.log('caught');
});

? Or do you mean my second potential solution aborts if an application set a custom unhandledRejection handler and that it's not desirable?

It solves the post mortem debugging issue but makes an opinionated choice on promise usage. It alters the behavior based on the flag but only a little.

Would you mind being a bit more specific? I think I understand what you mean and that's what I described in that same document I wrote, but I may be missing something. Also, if we're talking about the same issues, I think @chrisdickinson had a good point in one of his latest comments.

[spion]

@misterdjules your second option eliminates the ability to implement a using-like feature for managing resources like in Bluebird: http://bluebirdjs.com/docs/api/disposer.html

@benjamingr's insight is that we might not really need to use the microtask queue and unhandledRejection and we would still get the same functionality (except when user code throws rather than rejects in the promise constructor).

The code would look something like:

if (noCatchHandlerAttachedToThisPromise) { runCode() }
else { try { runCode() } catch { (forward to catch handler) }

Now if you avoid using catch or finally with promises, you pretty much get the same effects as if you avoid using catch in regular code. You get the same effect as your option 2, but its opt-in (not using promise.catch)

Its still kind of a stopgap measure, because ideally we want programmable filtered catch in the language and to simply do:

try { runCode() } catch (e if thisPromise.catchFilter) { (forward to catch handler) }

But I guess it will have to do for now, and we'll have to add a new reason why this shortcoming (lack of filtered catch) is unacceptable to the list of consideration to TC39.

And even more ideally we would have try version 2 as in swift, where you can also be explicit about the lines that you expect to throw:

attempt {
  try iDoExpectErrorsHere()
  iDontExpectExceptionsHere()
} catch (e if conditionFn) {
  ...
}

Of course disposers are implemented on top of finally, which negates the benefits above because it catches everything then executes code then rethrows... But I guess thats always true with any implementation of finally - isn't it? At least this way its up to the user to decide which parts warrant post-mortem debugging and which don't.

[misterdjules]

@spion

your second option eliminates the ability to implement a using-like feature for managing resources like in Bluebird: http://bluebirdjs.com/docs/api/disposer.html

Thank you for bringing that up, I was not aware of that feature!

we might not really need to use the microtask queue and unhandledRejection and we would still get the same functionality (except when user code throws rather than rejects in the promise constructor).

I'm not sure I understand what you mean. Do you mean we "might not really need to use the microtask queue" in general to implement promises?

The code would look something like:

if (noCatchHandlerAttachedToThisPromise) { runCode() } else { try { runCode() } catch { (forward to catch handler) }

Right, that's pretty much how my implementation works. By "the code", do you mean a separate implementation of promises made by subclassing global.Promise?

[spion]

@misterdjules I must be missing something, but I don't see the same thing it in your implementation. Given a promise p2, created by:

var p2 = p1.then(doSomethingThatCouldThrow);

The code that runs doSomethingThatCouldThrow, either on the next turn, or when p1 becomes available, would do something like this:

var resolution;
// in real code this flag would have to be updated and propagated by derived promises too
// e.g. if you did var p3 = p2.catch(...) or p4 = p2.then(...).catch(...) it would have to update.
if (p2._hasNoCatchHandlerRightNow) {
  resolution = doSomethingThatCouldThrow(p1._currentValue)
} else {
  try { resolution = doSomethingThatCouldThrow(p1._currentValue) }
  catch (e) { p2._forwardErrorToFirstErrorHandler(...); }
}
p2._resolveWith(resolution);

What I see in your implementation is a change of behaviour based on a global flag - this would instead change the behaviour based on whether the particular promise has a catch handler.

Then we can also try extending the promise class to have an .error method, a handler which is different from .catch and only deals with returned rejections.

I'm pretty sure all of this can be done as a userland library. With this library if you are not using promise.catch() in certain parts of your code, all thrown errors will end up in uncaughtException (not unhandledRejection) and the good old --abort-on-uncaught-exception will just work. Basically, the same rules as sync code

downsides:

1. need to figure out the promise constructor (a different, async execution of the passed resolve/reject callback must be used to give the consumer a chance to swoop in and attach a catch handler before we run the code)
2. async catch handler attachments are now strictly forbidden.

[petkaantonov]

Bluebird does pruning and does not give a complete stack trace (unlike chrome), and so misses recursive operations

If I turn off the duplication removal (which is obviously an intended feature) and run your example, I get exactly the same information except it comes with N lines of useless noise. This doesn't seem to be very useful so it never even occurred to me to make a configuration flag for this. I mean, just look at the stack you get with this feature disabled vs enabled. As you can see, it doesn't miss recursive operations, it removes duplication and noise.

[chrisdickinson]

Cross-linking my latest comment from the symposium repo. I'm going to

To frame things a bit: we are going to have to make modifications to V8 in any case, for exactly the reasons that @bmeck touches on — namely, we don't have enough control over the MicrotaskQueue for native Promises at present, and that breaks AsyncWrap, which we absolutely want to ship. I am currently working on this, and it's a blocker for both unflagging the promise API as well as exposing the AsyncWrap API.

In my linked comment, I explain that subclassing+patching Promise is an option of last resort for core — I tried to go down that road to fix asyncwrap and promises, and it's prohibitively difficult to do it without the leaking out details of the original native Promise. Absent patching Promise entirely, returning a specialized subclass from a Core Promise API is a no-go — this precludes use cases that we want to support, like swapping out the promise implementation at app level.

I believe I have come around to the point of view that Node should crash on unhandledRejection by default if no handler is installed, since existing package authors can't rely on their consumers not to crash the program on unhandledRejection anyway. This aligns unhandledRejection with uncaughtException, also.

I see two potential paths forward to implementing a "crash by default on unhandled rejection".

Approach 1

This approach is close to the status quo. It leaves users the ability to reject synchronously, at the expense of making the program behave differently under the --abort-on-unhandled-rejection flag — notably, with the flag enabled the program crashes earlier than it otherwise would.

1. A promise rejects and is unhandled.
2. If --abort-on-unhandled-rejection is not set,
   1. on nextTick, check to see if the promise is still rejected. If it is,
      1. Fire the unhandledRejection event. If a listener fires,
         1. continue normally.
      2. Otherwise,
         1. crash the program.
   2. If it isn't,
      1. continue normally.
3. Otherwise, if --abort-on-unhandled-rejection is set,
   1. immediately abort()
      • Assuming the PromiseHasUserDefinedRejectHandler changes from @misterdjules land in V8, the crash will happen at top of stack.

Approach 2

This approach aligns the behavior of Node running with and without the --abort-on-unhandled-rejection flag, at the expense of making synchronous rejection immediately crash the program. This could be mitigated somewhat by patching the Promise.reject helper to reject on next tick, but it still has some interesting repercussions for, e.g., programmer errors due to bad arguments passed to the Node API.

1. A promise rejects and is unhandled.
2. If --abort-on-unhandled-rejection is not set,
   1. Fire the unhandledRejection event. If a listener fires,
      1. continue normally.
   2. Otherwise,
      1. crash the program.
3. Otherwise, if --abort-on-unhandled-rejection is set,
   1. immediately abort()
      • Assuming the PromiseHasUserDefinedRejectHandler changes from @misterdjules land in V8, the crash will happen at top of stack.

The question is: is it acceptable to preclude catching of synchronous rejections? For example, the Node API will synchronously reject if called with bad parameters. Under this scheme, I believe there would be no way to catch that error in-situ — it would immediately crash the program unless a user installed a unhandledRejection handler. For example in:

try {
  const data = await fs.readFile(null)
} catch (err) {
  console.error('never reached')
}

We would never reach 'never reached' unless a unhandledRejection handler was installed. Is this acceptable? It seems to have some desirable elements for post mortem & error symposium users. My gut feeling is that synchronous rejection is enough of a corner case that it might be valuable to repurpose it in this fashion. On the other hand, a user running with --abort-on-uncaught-exception is likely to always run their application with the flag, so perhaps the slightly different behavior is worth the price of not having uncatchable, fatal exceptions by default?

/cc'ing @fishrock123 since IIRC he's looking into making unhandled rejections crash when the promise is GC'd. If we go with one of the approaches above we won't need to do that; however it would be good to get his input!

[spion]

@chrisdickinson what about @benjamingr's approach above, only catch errors in promises that already have a .catch handler added to them? That would

1. make --abort-on-uncaught-exception work with promises out of the box
2. cause the same rules to apply for both sync and async try-catch

@benjamingr you mentioned you have a prototype (fork of bluebird, I think) of this?

Regarding the GC approach, @petkaantonov mentioned that wont work when we were exploring that option for bluebird originally (if a long-running promise gets into the old space, it might not ever get reclaimed): https://github.com/petkaantonov/bluebird/issues/17

[spion]

To be more specific, approach 3 is:

1. The microtask queue is preparing to run a promise task.
2. If --abort-on-unhandled-rejection is not set,
   1. Run the task in a try-catch block. If exception/rejection occurs, and is not handled until next tick:
      1. Fire the unhandledRejection event. If a listener fires,
         1. continue normally.
      2. Otherwise,
         1. crash the program.
3. Otherwise, if --abort-on-unhandled-rejection is set
   1. Check if the promise has a .catch handler attached. (1) If not,
      1. Run the task without a try-catch block.
      2. If the task throws, this will cause an uncaughtException
      3. Abort is called, core dump with correct stack is generated.
   2. Otherwise (promise already has a .catch handler)
      1. Run the task in a try-catch block.
      2. If the task throws, propagate exception normally.
      3. Abort is not called, analogous to when a sync try-catch block is encountered.

(1): this would be a flag that propagates in the promise tree from children to parents.

edit: @benjamingr do you have any idea how to make this work with async/await?

[chrisdickinson]

@spion I believe we've described very nearly the same thing — approach 3 is exactly approach 1! :) In particular — in both "approach 1" and "approach 2" the run with/without try/catch block bits are what I'm referring to in regards to @misterdjules work (in that 3.i.· section in both cases). We're describing it from different angles: the two approaches I listed are from the perspective of "an error has occurred." In retrospect that's pretty confusing since @misterdjules's "try/catch only if there's a handler in --abort mode" has to happen before the fact. I'll clean these up tonight in a followup comment.

With regards to the crash-on-rejected-GC approach, I bring it up because the approaches listed here would take the place of that approach.

[spion]

@chrisdickinson Oh, I see it now in the PR (PromiseHasUserDefinedRejectHandler). I managed to miss it two times, sorry!

Will this work with generators or async await, though? There is no way to query a generator iterator whether its presently running within a catch block.

[trevnorris]

Check if the promise has a .catch handler attached.

Can you help me fill in a blank (sorry, jumping in late and tried to get through as much of the thread as possible). The way I read this it says "when the Promise executor runs and throws an exception, check if a .catch() is attached." But this will always be false because the executor will run before the script has been allowed to attach the handler. Or are we saying after the microtask queue has attempted to call the handler? I also saw mention of running the executor asynchronously. Mind clarifying this for me?

[vkurchatkin]

Can anyone try this patch?

diff --git a/deps/v8/src/isolate.cc b/deps/v8/src/isolate.cc
index 04198bb..575beb6 100644
--- a/deps/v8/src/isolate.cc
+++ b/deps/v8/src/isolate.cc
@@ -1001,8 +1001,21 @@ Object* Isolate::Throw(Object* exception, MessageLocation* location) {
     debug()->OnThrow(exception_handle);
   }

+  Handle<Object> promise = GetPromiseOnStackOnThrow();
+  bool possible_uncaught_promise = false;
+
+  if (promise->IsJSObject()) {
+    Handle<JSObject> jspromise = Handle<JSObject>::cast(promise);
+
+    Handle<Object> has_reject_handler;
+    ASSIGN_RETURN_ON_EXCEPTION_VALUE(
+        this, has_reject_handler,
+        debug()->PromiseHasUserDefinedRejectHandler(jspromise), nullptr);
+    possible_uncaught_promise = has_reject_handler->IsFalse();
+  }
+
   // Generate the message if required.
-  if (requires_message && !rethrowing_message) {
+  if ((requires_message && !rethrowing_message) || possible_uncaught_promise) {
     MessageLocation computed_location;
     // If no location was specified we try to use a computed one instead.
     if (location == NULL && ComputeLocation(&computed_location)) {
@@ -1025,7 +1038,8 @@ Object* Isolate::Throw(Object* exception, MessageLocation* location) {
       // or if the custom callback determined that V8 should abort, then
       // abort.
       if (FLAG_abort_on_uncaught_exception &&
-          PredictExceptionCatcher() != CAUGHT_BY_JAVASCRIPT &&
+          (PredictExceptionCatcher() != CAUGHT_BY_JAVASCRIPT ||
+            possible_uncaught_promise) &&
           (!abort_on_uncaught_exception_callback_ ||
            abort_on_uncaught_exception_callback_(
                reinterpret_cast<v8::Isolate*>(this)))) {

I've tried to inspect resulting cores with llnode and they look as expected to me (although I've only checked bt)

[spion]

@trevnorris Since promises guarantee to execute anything passed to a then or catch handler in the next turn (after the current execution context), in this example:

p.then(function okHandler(val) { throw new Error("Error!"); })
.catch(someErrorHandler);

even if p is already resolved, okHandler is guaranteed to run after .catch by the microtask queue.

The only remaining problematic code is the promise constructor, which executes synchronously. We'd need a new way to construct:

function mkPromise() {
  return Promise.asyncConstructor((resolve, reject) => {
    // this will execute on the next turn to give a chance 
    // to the caller getting the value returned by `mkPromise` to attach an error handler
  });
}

[vkurchatkin]

Here are some results:

function bar() {
  throw new Error('boom');
}

function foo() {
  bar();
}

function boom() {
  return new Promise(function() {
    foo();
  });
}

boom();

Stack trace:

 * thread #1: tid = 0x0000, 0x000000010073a4a2 node`v8::base::OS::Abort() + 18 at platform-posix.cc:229, stop reason = signal SIGSTOP
  * frame #0: 0x000000010073a4a2 node`v8::base::OS::Abort() + 18 at platform-posix.cc:229
    frame #1: 0x000000010044afde node`v8::internal::Isolate::Throw(this=<unavailable>, exception=<unavailable>, location=<unavailable>) + 686 at isolate.cc:1053
    frame #2: 0x000000010056365f node`v8::internal::Runtime_Throw(int, v8::internal::Object**, v8::internal::Isolate*) [inlined] v8::internal::__RT_impl_Runtime_Throw(isolate=0x0000000101805000) + 33 at runtime-internal.cc:82
    frame #3: 0x000000010056363e node`v8::internal::Runtime_Throw(args_length=<unavailable>, args_object=<unavailable>, isolate=0x0000000101805000) + 14 at runtime-internal.cc:79
    frame #4: 0x000028655b10b61b <internal code>
    frame #5: 0x000028655b24b21c bar(this=0x00001e74877046b1:<unknown>) at /Users/vkurchatkin/io.js/tmp.js:1:13 fn=0x00003c97be0ab469
    frame #6: 0x000028655b24b0f2 foo(this=0x00001e74877046b1:<unknown>) at /Users/vkurchatkin/io.js/tmp.js:5:13 fn=0x00003c97be0ab4b1
    frame #7: 0x000028655b24b012 (anonymous)(this=0x00001e74877046b1:<unknown>) at /Users/vkurchatkin/io.js/tmp.js:10:30 fn=0x00003c97be0ab5b1
    frame #8: 0x000028655b10f3da <adaptor>
    frame #10: 0x000028655b134f55 <constructor>
    frame #11: 0x000028655b24a4e7 boom(this=0x00001e74877046b1:<unknown>) at /Users/vkurchatkin/io.js/tmp.js:9:14 fn=0x00003c97be0ab4f9
    frame #12: 0x000028655b24a022 (anonymous)(this=0x00003c97be0a8911:<Object: Object>, 0x00003c97be0a8911:<Object: Object>, 0x00003c97be0aafa1:<function: require at internal/module.js:12:19>, 0x00003c97be0a8819:<Object: Module>, 0x00003c97be0a67b1:<String: "/Users/vkurchatk...">, 0x00003c97be0aae81:<String: "/Users/vkurchatk...">) at /Users/vkurchatkin/io.js/tmp.js:0:10 fn=0x00003c97be0aae21
    frame #13: 0x000028655b136163 <internal>
    frame #14: 0x000028655b249861 Module._compile(this=0x00003c97be0a8819:<Object: Module>, 0x00003c97be0aa9a9:<String: "
function bar() ...">, 0x00003c97be0a67b1:<String: "/Users/vkurchatk...">) at module.js:379:37 fn=0x0000118c5673be79
    frame #15: 0x000028655b24306b Module._extensions..js(this=0x00003c97be0a3651:<Object: Object>, 0x00003c97be0a8819:<Object: Module>, 0x00003c97be0a67b1:<String: "/Users/vkurchatk...">) at module.js:423:37 fn=0x0000118c5673bf21
    frame #16: 0x000028655b240b8f Module.load(this=0x00003c97be0a8819:<Object: Module>, 0x00003c97be0a67b1:<String: "/Users/vkurchatk...">) at module.js:347:33 fn=0x0000118c5673bd49
    frame #17: 0x000028655b236b52 Module._load(this=0x00003c97be0619a9:<function: Module at module.js:39:16>, 0x00003c97be05d2e1:<String: "/Users/vkurchatk...">, 0x000006d7f3f04101:<null>, 0x000006d7f3f04231:<true>) at module.js:284:24 fn=0x0000118c5673bbc1
    frame #18: 0x000028655b236546 Module.runMain(this=0x00003c97be0619a9:<function: Module at module.js:39:16>) at module.js:448:26 fn=0x0000118c5673c119
    frame #19: 0x000028655b1431b9 startup(this=0x000006d7f3f04189:<undefined>) at node.js:12:19 fn=0x00001e7487704621
    frame #20: 0x000028655b13db75 (anonymous)(this=0x00001e74877046b1:<unknown>, 0x00001e7487704541:<Object: process>) at node.js:9:10 fn=0x00001e7487704669
    frame #21: 0x000028655b137044 <internal>
    frame #22: 0x000028655b11a8e2 <entry>
    frame #23: 0x000000010037e2da node`v8::internal::(anonymous namespace)::Invoke(isolate=0x0000000000000001, is_construct=<unavailable>, target=<unavailable>, receiver=<unavailable>, argc=1, args=<unavailable>, new_target=<unavailable>) + 410 at execution.cc:98
    frame #24: 0x000000010037dfd0 node`v8::internal::Execution::Call(isolate=<unavailable>, callable=<unavailable>, receiver=<unavailable>, argc=<unavailable>, argv=<unavailable>) + 464 at execution.cc:167
    frame #25: 0x0000000100147599 node`v8::Function::Call(this=0x000000010184a818, context=<unavailable>, recv=<unavailable>, argc=1, argv=0x00007fff5fbfef38) + 281 at api.cc:4401
    frame #26: 0x00000001006b6f8b node`node::LoadEnvironment(env=0x0000000101849000) + 486 at node.cc:3226
    frame #27: 0x00000001006b8388 node`node::Start(int, char**) + 313 at node.cc:4157
    frame #28: 0x00000001006b824f node`node::Start(argc=<unavailable>, argv=<unavailable>) + 189 at node.cc:4246
    frame #29: 0x0000000100001034 node`start + 52

Trying to catch:

function bar() {
  throw new Error('boom');
}

function foo() {
  bar();
}

function boom() {
  return new Promise(function() {
    foo();
  });
}

boom().catch(function(){});

Still dumps core, as handler is attached later (as expected).

Explicilty rejecting:

function bar() {
  throw new Error('boom');
}

function foo() {
  bar();
}

function boom() {
  return new Promise(function(resolve, reject) {
    reject();
  });
}

boom();

No effect, explicitly rejecting (or returning reject promise) is not resulted in abort (as expected).

Asynchronously throwing inside of a handler:

function bar() {
  throw new Error('boom');
}

function foo() {
  bar();
}

function boom() {
  return new Promise(function(resolve, reject) {
    setImmediate(resolve);
  }).then(function() {
    foo();
  });
}

boom();

Stack trace:

 * thread #1: tid = 0x0000, 0x000000010073a4a2 node`v8::base::OS::Abort() + 18 at platform-posix.cc:229, stop reason = signal SIGSTOP
  * frame #0: 0x000000010073a4a2 node`v8::base::OS::Abort() + 18 at platform-posix.cc:229
    frame #1: 0x000000010044afde node`v8::internal::Isolate::Throw(this=<unavailable>, exception=<unavailable>, location=<unavailable>) + 686 at isolate.cc:1053
    frame #2: 0x000000010056365f node`v8::internal::Runtime_Throw(int, v8::internal::Object**, v8::internal::Isolate*) [inlined] v8::internal::__RT_impl_Runtime_Throw(isolate=0x0000000101805000) + 33 at runtime-internal.cc:82
    frame #3: 0x000000010056363e node`v8::internal::Runtime_Throw(args_length=<unavailable>, args_object=<unavailable>, isolate=0x0000000101805000) + 14 at runtime-internal.cc:79
    frame #4: 0x0000058dd7b0b61b <internal code>
    frame #5: 0x0000058dd7c50edc bar(this=0x00001e18107046b1:<unknown>) at /Users/vkurchatkin/io.js/tmp.js:1:13 fn=0x00000c8b240ab4e9
    frame #6: 0x0000058dd7c50db2 foo(this=0x00001e18107046b1:<unknown>) at /Users/vkurchatkin/io.js/tmp.js:5:13 fn=0x00000c8b240ab531
    frame #7: 0x0000058dd7c50cd2 (anonymous)(this=0x00001e18107046b1:<unknown>) at /Users/vkurchatkin/io.js/tmp.js:12:19 fn=0x00000c8b240abe71
    frame #8: 0x0000058dd7b0f3da <adaptor>
    frame #12: 0x0000058dd7b37044 <internal>
    frame #13: 0x0000058dd7b1a8e2 <entry>
    frame #14: 0x000000010037e2da node`v8::internal::(anonymous namespace)::Invoke(isolate=0x0000000000000001, is_construct=<unavailable>, target=<unavailable>, receiver=<unavailable>, argc=0, args=<unavailable>, new_target=<unavailable>) + 410 at execution.cc:98
    frame #15: 0x000000010037dfd0 node`v8::internal::Execution::Call(isolate=<unavailable>, callable=<unavailable>, receiver=<unavailable>, argc=<unavailable>, argv=<unavailable>) + 464 at execution.cc:167
    frame #16: 0x000000010037e4ea node`v8::internal::Execution::TryCall(isolate=0x0000000101805000, callable=Handle<v8::internal::Object> @ r14, receiver=Handle<v8::internal::Object> @ r13, argc=0, args=0x0000000000000000, exception_out=<unavailable>) + 122 at execution.cc:205
    frame #17: 0x0000000100450b04 node`v8::internal::Isolate::RunMicrotasks(this=0x0000000101805000) + 388 at isolate.cc:2703
    frame #18: 0x00000001006a6689 node`node::Environment::KickNextTick(this=0x0000000101849000) + 43 at env.cc:75
    frame #19: 0x00000001006b1174 node`node::MakeCallback(env=0x0000000101849000, recv=<unavailable>, callback=<unavailable>, argc=0, argv=0x0000000000000000) + 745 at node.cc:1206
    frame #20: 0x00000001006b85ce node`node::CheckImmediate(handle=<unavailable>) + 98 at node.cc:210
    frame #21: 0x000000010072bf2f node`uv__run_check(loop=0x0000000100be37f0) + 167 at loop-watcher.c:67
    frame #22: 0x0000000100727537 node`uv_run(loop=0x0000000100be37f0, mode=UV_RUN_ONCE) + 329 at core.c:352
    frame #23: 0x00000001006b83d3 node`node::Start(int, char**) + 388 at node.cc:4170
    frame #24: 0x00000001006b824f node`node::Start(argc=<unavailable>, argv=<unavailable>) + 189 at node.cc:4246
    frame #25: 0x0000000100001034 node`start + 52
(lldb) 

Attaching error handler:

function bar() {
  throw new Error('boom');
}

function foo() {
  bar();
}

function boom() {
  return new Promise(function(resolve, reject) {
    setImmediate(resolve);
  }).then(function() {
    foo();
  }).then(function() {
    bar()
  }).catch(function() {
  });
}

boom();

Exits cleanly, as expected.

[spion]

@vkurchatkin what about

function boom() {
  Promise.resolve().then(foo);
}

(should abort with correct trace)

Also

function boom() {
  Promise.resolve().then(foo).catch(_ => _)
}

should not abort

[vkurchatkin]

First one dumps core, this second one doesn't, as foo is executed asynchronously after catch is called

[spion]

@vkurchatkin awesome!

[benjamingr]

Awesome, @misterdjules @chrisdickinson @groundwater pinging you to take a look at @vkurchatkin's patch at https://github.com/nodejs/post-mortem/issues/16#issuecomment-183323420

[misterdjules]

@vkurchatkin I am not sure the patch you mentioned is a viable approach. Consider this sample code:

$ cat /tmp/test-exception-from-promise-caught.js
function bar() {
  throw new Error('boom');
}

function foo() {
  bar();
}

function boom() {
  return new Promise(function() {
    foo();
  });
}

try {
  boom();
} catch (e) {
  console.log('caught');
}
$

Now running this sample code with your proposed change and passing the --abort-on-uncaught-exception command line I get this:

$ ./node --abort-on-uncaught-exception /tmp/test-exception-from-promise-caught.js
Uncaught Error: boom

FROM
bar (/private/tmp/test-exception-from-promise-caught.js:2:3)
foo (/private/tmp/test-exception-from-promise-caught.js:6:3)
/private/tmp/test-exception-from-promise-caught.js:11:5
boom (/private/tmp/test-exception-from-promise-caught.js:10:10)
Object.<anonymous> (/private/tmp/test-exception-from-promise-caught.js:16:3)
Module._compile (module.js:417:34)
Object.Module._extensions..js (module.js:426:10)
Module.load (module.js:357:32)
Function.Module._load (module.js:314:12)
Function.Module.runMain (module.js:451:10)
startup (node.js:149:18)
node.js:1013:3
[1]    65816 illegal hardware instruction  ./node --abort-on-uncaught-exception 
$ 

This sample code should not abort when run with --abort-on-uncaught-exception.

I believe Isolate::Throw is not the right place where to make changes to implement aborting/exiting on an uncaught error thrown from within a promise. Isolate::Throw should just be concerned by whether there's a try/catch handler in the stack above the current stack frame.

Instead, I would recommend exploring the approach I took with the potential solution I already presented in this issue, which behaves as expected (it doesn't abort) when running the sample code above. Basically, I think Isolate::RunMicrotasks and the promises builtins are where we need to implement that change.

Please note that the potential solution I mention above is a prototype/proof of concept though, and there are likely more changes/adjustments to make before it's ready to consider as a solid change. For instance, we might want to introduce a new command line flag instead of conflating that behavior --abort-on-uncaught-exception's behavior, so that e.g a user could make node exit (not abort) when using that flag but not using --abort-on-uncaught-exception.

Obviously, I may have missed a few things in that potential solution, so I would appreciate if you could take a look at it and let me know what you think.

I hope that helps.

[benjamingr]

@misterdjules For what it's worth - I'd definitely expect it to abort on a throw in the promise constructor - that's almost always a programmer error and almost never an operational one. So much, that I'm absolutely 100% convinced that in the last three years answering thousands of questions in StackOverflow, GitHub and chat I've never seen an intentional use case of this.

Not aborting here is dangerous, the promise constructor already converts thrown errors to rejections. Throwing outside of it would be a severe change to the program flow. Aborting terminates the program and thus does not introduce different branching.

Having the promise constructor behave differently and cause different flow control based on a flag is something that will generate a lot more objections. You raised this concern yourself first. @vkurchatkin 's solution does not have that problem.

[vkurchatkin]

@misterdjules

Isolate::Throw should just be concerned by whether there's a try/catch handler in the stack above the current stack frame.

The problem is that there is ALWAYS try/catch handler in the stack above in case of promises, so we have to special case it .

This sample code should not abort when run with --abort-on-uncaught-exception.

This is debatable. This patch doesn't allow to throw out of promise handlers and executor regardless of outer try catches. The semantics is weird, indeed, but try-catch can't affect this: promise constructor doesn't throw anything ever.

Instead, I would recommend exploring the approach I took with the potential solution I already presented in this issue, which behaves as expected (it doesn't abort) when running the sample code above.

Thanks, will take a look.

For instance, we might want to introduce a new command line flag

Most certainly, for compatibility reasons.

@benjamingr

I'd definitely expect it to abort on a throw in the promise constructor - that's almost always a programmer error and almost never an operational one.

That's the idea. People who want to use this functionality should avoid throw inside of promise handlers and executor explicitly and use return Promise.reject instead. All errors left should be programmer errors and bugs.

[benjamingr]

@vkurchatkin actually, I think that this part:

People who want to use this functionality should avoid throw inside of promise handlers and executor explicitly and use return Promise.reject instead.

Isn't really necessary. It is only required for the promise constructor which you aren't supposed to use except when converting APIs to promises.

In actual then handlers no such caution is necessary and you can safely keep throwing because the code inside the then is always executed asynchronously and the catch handlers would have been attached already anyway.

This is what @spion asked about and replied "awesome" for.

Basically, I can keep doing:

Promise.resolve().then(v => {
    throw new Error();
}).catch(e => {
   // ...
});

and not get any false positives.

[benjamingr]

More details here: https://gist.github.com/benjamingr/3fb97eda723ff2a030d1

[vkurchatkin]

@benjamingr that's the point, people who want core dumps shouldn't have catch handlers at all

@misterdjules your approach is interesting, but it clearly violates standard. Also I think not catching errors from microtasks can have unforeseen consequences.

[benjamingr]

@vkurchatkin I don't understand, with your patch you have both - you can have .catch handlers where you want to recover from errors and if you omit it you still get programmer errors with full stack traces.

The only compromise is that now catch handlers can cause false positives for a core dump and we're basically forcing an opinionated take on how people should write their promise code for core dumps.

I think it's a good path to explore.

[vkurchatkin]

were you want to recover

But this way you might accidentally catch a programmer error.

The only compromise is that now catch handlers can cause false positives for a core dump

No, core is dumped only when there are no catch handlers, but this can be easily changed