src: fix MakeCallback error handling

[trevnorris]

The final goal of this PR is to allow MakeCallback (in both node:: and AsyncWrap::) to be reentrant. Reason is because the HTTPParser may enter the JS call stack initially, or be called while already in a stack (see @indutny's PR #4509). Also so native addon developers can always use node::MakeCallback without the worry of code being executed in the wrong order (specifically in relation to usage of nextTick or the MicrotaskQueue).

To do this I first made sure the two implementations of MakeCallback were the same. Previously they would return at different points in the case of caught exception.

Finally was the addition of AsyncCallbackScope (admittedly the concept was taken from Fedor's PR above) to allow MakeCallback to be called recursively without each subsequent call from processing the nextTickQueue or the MicrotaskQueue.

I am working with @misterdjules on the proper way of handling domains. So this PR is currently a WIP.

Ref: nodejs/node-v0.x-archive#9245

R=@bnoordhuis
R=@indutny

[bnoordhuis]

/cc @bajtos

[indutny]

Why SetVerbose doesn't allow V8 going to native again?

[trevnorris]

@indutny Haven't investigated as to why that's the case. Just noticed it while testing. Could be a bug in v8? Header documentation doesn't indicate that control will never return to native.

[indutny]

@trevnorris indeed, it should just capture the error. What exactly do you mean by "control will never return to native"?

[trevnorris]

@indutny What I mean is that code execution will never return to the native API. IIRC (will have to double check) but I believe this is even the case if uncaughtException is used.

[trevnorris]

This PR is incorrect. The try_catch will be overridden if domain.exit() or async post callbacks are called. Guess we store the error information and manually throw it later. Will give that a shot.

[trevnorris]

@misterdjules /cc you on this since domains are acting all strange. will post more info and would like whatever feedback you may have.

[misterdjules]

If try_catch.SetVerbose(true) was called, wouldn't FatalException be called anyway?

[trevnorris]

@misterdjules Here's the diff to demonstrate the issue:

diff --git a/src/async-wrap.cc b/src/async-wrap.cc
index c9f5caa..fbff146 100644
--- a/src/async-wrap.cc
+++ b/src/async-wrap.cc
@@ -216,10 +216,6 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,

   Local<Value> ret = cb->Call(context, argc, argv);

-  if (try_catch.HasCaught()) {
-    return Undefined(env()->isolate());
-  }
-
   if (ran_init_callback() && !post_fn.IsEmpty()) {
     try_catch.SetVerbose(false);
     post_fn->Call(context, 0, nullptr);
@@ -237,6 +233,10 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
     }
   }

+  if (try_catch.HasCaught()) {
+    return Undefined(env()->isolate());
+  }
+
   Environment::TickInfo* tick_info = env()->tick_info();

   if (tick_info->in_tick()) {

And here's the script:

'use strict';
const async_wrap = process.binding('async_wrap');
const domain = require('domain');
const crypto = require('crypto');
const d = domain.create();

function noop() { }

async_wrap.setupHooks(noop, noop, noop);
async_wrap.enable();

d.on('error', function() { });

d.run(function() {
  crypto.randomBytes(4, function() { throw new Error('poo') });
});

This causes the application to abort. This is because the try_catch around cb->Call() throws, and since we never run try_catch.Reset() it causes the async wrapp post callback to be seen as having thrown. Thus aborting.

Demonstrating that the node::MakeCallback() impl current has an issue but isn't manifested in core b/c it isn't consumed by core in a way that would trigger it. Also that there's a difference in how the two impl of MakeCallback work. This intends to make AsyncWrap::MakeCallback return if an error is caught after the asyncwrap and domain exit callbacks have had a chance to run.

[misterdjules]

If I understand correctly, try_catch.HasCaught() will always return false after a call to TryCatch::Reset, so I'm not sure I understand what the use case is.

[misterdjules]

@trevnorris OK, I think I understand the intent of this PR. Can you please update the original description of this PR + the commit message according to what you described in your latest comment? Otherwise understanding the intent of this PR is very difficult.

Now I also understand better one of the questions you had in #node-dev:

so SetVerbose() prevents returning to C++, which means domain->exit() wasn't running if the callback threw. is this expected?

which I interpret now as:

When a callback running within a domain throws an error, is Domain.prototype.exit for the current active domain expected to run?

I would think that the best one can do in this case is to either:

1. Have a domain error handler that does some cleanup and exit as soon as possible.
2. Have a process 'uncaughtException' event handler that does some cleanup and exit as soon as possible.
3. Abort and generate a core dump.
4. Simply exit with an error message that describes the error that was thrown.

In other words, one should not expect the node runtime to be able to continue its execution normally.

Moreover, when a domain error handler is called, all domains on the stack are cleared. So I'm not sure it's useful or even if it makes sense to call Domain.prototype.exit when an error is thrown within a domain.

I'm not familiar with the AsyncWrap subsystem, but I'd also be interested to know what the semantics are regarding post hooks and errors thrown within an async callback. Do we consider the async operation to be done, or failed? Do we make a distinction between the two?

Basically, I'm wondering if the current AsyncWrap::MakeCallback implementation (i.e not calling domain.exit() and not calling the post asyncwrap hook) should not also be reflected in node::MakeCallback and anywhere else a callback is run within a domain.

[bajtos]

The main purpose of adding try_catch.SetVerbose was to enable Node Inspector to break on uncaught errors at the point where/when such error was thrown - see nodejs/node-v0.x-archive#5713 and nodejs/node-v0.x-archive#6460.

I vaguely remember that "break on uncaught error" may have stopped working in later v0.11 versions, i.e. it's possible it worked only in few v0.11 versions and never else.

Just my two cents to give a bit more context, I am afraid I don't have enough time to take a deeper look at this anytime soon.

[trevnorris]

@misterdjules My original comment about SetVerbose() preventing return of code execution to C++ was completely off. I didn't realize that in the case of an uncaught exception the application would exit() immediately.

Moreover, when a domain error handler is called, all domains on the stack are cleared. So I'm not sure it's useful or even if it makes sense to call Domain.prototype.exit when an error is thrown within a domain.

This is important. So are you saying it's not necessary to call the domain's exit() callback from MakeCallback when an error is thrown in a domain's scope?

I'm not familiar with the AsyncWrap subsystem, but I'd also be interested to know what the semantics are regarding post hooks and errors thrown within an async callback.

If an async wrap callback throws the application aborts. I have no idea what the application of the state could be in this case, and since it's currently a "private" API wanted to make sure this never happened.

Basically, I'm wondering if the current AsyncWrap::MakeCallback implementation (i.e not calling domain.exit() and not calling the post asyncwrap hook) should not also be reflected in node::MakeCallback and anywhere else a callback is run within a domain.

If it's the case that the domain's exit() callback doesn't need to be called then this sounds about right. Though I may allow the async wrap post callback to run regardless for error reporting, but that's not part of this conversation.

@bajtos Thanks for the info. With that context now I'll know what to look for. Nevermind my concerns about SetVerbose(). It's not hurting anything as it is so not worried about it.

[trevnorris]

CI: https://ci.nodejs.org/job/node-test-pull-request/1147/

[joaocgreis]

@trevnorris the windows failure was due to work in progress in CI. Relaunched here: https://ci.nodejs.org/job/node-test-commit-windows-fanned/815/

[misterdjules]

@trevnorris I still don't understand what this PR intends to fix in terms of error handling. Is the purpose of this PR to fix the issue described in https://gist.github.com/misterdjules/fac76caa775b3ad63ac8?

[trevnorris]

@misterdjules This PR has shifted a couple times now as I've been working on it. The wording of "fix error handling" is meant to be "fix timing of return in MakeCallback on caught exception". And this commit really was only meant as cleanup for the following commit, making sure MakeCallback is reentrant. I'll reword the OP to reflect this.