src,async_hooks,n-api: refactor async callback handling

[addaleax]

• Refactor how asynchronous code is executed from within Node
• Merge the two almost-but-not-quite identical MakeCallback() implementations
• Provide a public CallbackScope class for embedders in order to enable MakeCallback()-like behaviour without tying that to calling a JS function
• Enable combining N-API async work with async-hooks

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

Affected core subsystem(s)

/cc @nodejs/async_hooks
/cc @nodejs/n-api (Who may or may not only want to review the N-API commit; this is a breaking N-API change)

[addaleax]

CI: https://ci.nodejs.org/job/node-test-commit/11632/
CI: https://ci.nodejs.org/job/node-test-commit/11633/
CI: https://ci.nodejs.org/job/node-test-commit/11634/
CI: https://ci.nodejs.org/job/node-test-commit/11635/
CI: https://ci.nodejs.org/job/node-test-commit/11636/
CI: https://ci.nodejs.org/job/node-test-commit/11637/

[jasongin]

N-API parts look good.

I'm still planning to add an async context parameter to napi_make_callback() to support other async scenarios that don't use the N-API async work APIs.

If I'm understanding correctly, the change in this PR means that when the N-API async work APIs are used, during the napi_async_complete_callback invocation the current async context is already correct. Usually that callback will then invoke napi_make_callback(). So the additional async context parameter to napi_make_callback() can be optional, and the current async context used if another context is not specified.

[addaleax]

@jasongin This PR would mean that napi_make_callback would be unnecessary for most completion C callbacks; N-API users who use the async work API wouldn’t need to bother with any of the async stuff and could just use a plain function call.

[jasongin]

Oh, so the async work callback should just call napi_call_function() instead? OK, that's simpler.

Then the only need for napi_make_callback() (updated with async context parameter) will be for async code that doesn't use the async work API.

[jasongin]

In that case should test/addons-napi/test_async/test_async.cc be updated to call napi_call_function() instead of napi_make_callback() (in 2 places)?

[addaleax]

@jasongin Exactly 👍

In that case should test/addons-napi/test_async/test_async.cc be updated to call napi_call_function() instead of napi_make_callback() (in 2 places)?

Right. Done!

[addaleax]

Rebased … maybe somebody of @AndreasMadsen @trevnorris @refack @TimothyGu @tniessen @bnoordhuis @jasnell could be reviewer (if only for the yet-unreviewed non-N-API part)?

CI: https://ci.nodejs.org/job/node-test-commit/11769/

[thefourtheye]

Nit: exposed?

[addaleax]

Right, fixed.

[addaleax]

@trevnorris I think all your comments have been addressed or answered so far. Since others in the N-API team have expressed the desire – and I agree – for the breaking N-API changes to get out sooner rather than later, I would like to have the option of landing this soon so it can go into 8.5.0 (unless anybody sees glaring issues with it). That doesn’t mean we can’t make modifications to the internals later. If you think this is a terrible idea, I’d open a PR that contains just the breaking API changes of this one without any wiring behind them.

CI: https://ci.nodejs.org/job/node-test-commit/12286/

[trevnorris]

@addaleax Mind verifying some of the following implementation details?

In test/addons/callback-scope/binding.cc there's the following:

  node::CallbackScope(isolate, args[0].As<v8::Object>(), asyncContext);
  v8::Local<v8::Function> fn = args[3].As<v8::Function>();

  v8::MaybeLocal<v8::Value> ret =
      fn->Call(isolate->GetCurrentContext(), args[0], 0, nullptr);

Which appears to indicate that the user can always call v8::Function::Call() inside the scope of a node::CallbackScope. Though I don't believe this is the case. Stepping though a possible scenario:

• Call node::CallbackScope()
• Which calls node::InternalCallbackScope()
• Which attempts a node::DomainEnter()
• If the domain is disposed then node::InternalCallbackScope::failed_ is set to true and returns early
• Thus skipping the call to node::AsyncWrap::EmitBefore() and node::Environment::AsyncHooks::push_ids().
• The user calls v8::Function::Call() on the callback unaware of the fact, and unable to observe, that no asynchronous state has been setup
• Because the domain has been disposed the nextTickQueue is not processed on ~InternalCallbackScope.

So there's a scenario where both async_hooks and the nextTickQueue won't be invoked as expected, and the user has no way to know this.

[trevnorris]

@addaleax The following is a discrepancy between using node::CallbackScope and node::MakeCallback:

C++ code for test module

#include <v8.h>
#include <node.h>

namespace bmod {

using namespace v8;


static void RunCallbackScope(const FunctionCallbackInfo<Value>& args) {
  assert(args[0]->IsNumber());
  assert(args[1]->IsNumber());
  assert(args[2]->IsFunction());
  assert(args[3]->IsObject());

  auto isolate = args.GetIsolate();
  auto ctx = isolate->GetCurrentContext();
  double a_id = args[0]->NumberValue();
  double t_id = args[1]->NumberValue();
  auto fn = args[2].As<Function>();
  auto obj = args[3].As<Object>();

  node::CallbackScope(isolate, obj, { a_id, t_id });

  auto ret = fn->Call(ctx, obj, 0, nullptr);

  if (ret.IsEmpty()) {
    fprintf(stderr, "fn call failed\n");
  }
}


static void RunMakeCallback(const FunctionCallbackInfo<Value>& args) {
  assert(args[0]->IsNumber());
  assert(args[1]->IsNumber());
  assert(args[2]->IsFunction());
  assert(args[3]->IsObject());

  auto isolate = args.GetIsolate();
  double a_id = args[0]->NumberValue();
  double t_id = args[1]->NumberValue();
  auto fn = args[2].As<Function>();
  auto obj = args[3].As<Object>();

  auto ret = node::MakeCallback(isolate, obj, fn, 0, nullptr, { a_id, t_id });

  if (ret.IsEmpty()) {
    fprintf(stderr, "fn call failed\n");
  }
}


void Init(Local<Object> exports) {
  NODE_SET_METHOD(exports, "runMakeCallback", RunMakeCallback);
  NODE_SET_METHOD(exports, "runCallbackScope", RunCallbackScope);
}

}  // namespace bmod

NODE_MODULE(addon, bmod::Init)

Here is the JS that uses the source above:

'use strict';

const release_type = process.config.target_defaults.default_configuration;
const { runCallbackScope, runMakeCallback } = require(`./build/${release_type}/addon`);
const async_hooks = require('async_hooks');
const { executionAsyncId, triggerAsyncId } = async_hooks;
const print = process._rawDebug;

async_hooks.createHook({
  before(id) { print('before: ', id); },
  after(id) { print('after:  ', id); },
}).enable();

const obj = {};

function fn() {
  print(`asyncId: ${executionAsyncId()}\ttriggerId: ${triggerAsyncId()}`);
}

runCallbackScope(42, 43, fn, obj);
runMakeCallback(42, 43, fn, obj);

Output from the test:

before:  42
after:   42
asyncId: 1      triggerId: 0
before:  42
asyncId: 42     triggerId: 43
after:   42

Issue is the global values returned by executionAsyncId() and triggerAsyncId() while the function is executing. I haven't figured out why this is happening yet, but will look further into it.

[addaleax]

@trevnorris That latter problem is a typo in the test file; I fixed it here now. You need to give the CallbackScope a name so that it’s not a temporary, but actually expands over the rest of the current scope.

Re: Disposed domains … sigh. We should remove those in Node 10. But you’re right, the API should provide for that; I’m thinking something along scope.IsDomainDisposed() for users to be able to tell whether that situation occurred or not.

[trevnorris]

@addaleax

That latter problem is a typo in the test file

Whoops. Thanks.

Disposed domains … sigh. We should remove those in Node 10.

We might just make the case that supporting a deprecated API in a deprecated module isn't necessary. Especially if it means support requires an additional API. Though not sure who we'd need to confirm that with.

[addaleax]

Disposed domains … sigh. We should remove those in Node 10.

We might just make the case that supporting a deprecated API in a deprecated module isn't necessary. Especially if it means support requires an additional API. Though not sure who we'd need to confirm that with.

@trevnorris I would agree :) I would be fine with just seeing if anybody complains. After all, .dispose() has been runtime-deprecated since d86814a (that’s over 4 years!)

[addaleax]

i.e. I’ll open a semver-major PR to remove it once this lands.

[addaleax]

Fresh CI: https://ci.nodejs.org/job/node-test-commit/12337/

[addaleax]

Rebased due to conflicts with 35a526c, one more CI: https://ci.nodejs.org/job/node-test-commit/12360/

I would like to land this if it comes back green

[addaleax]

Landed in 4ae0afb...1a0727d

@trevnorris Feel free to add more comments/ask questions, I’d be happy to address those in subsequent PRs; at this point, I think the PR is quite plainly ready, and it’s good to get the last batch of breaking N-API commits out in the next release.

[trevnorris]

@addaleax looks good. thanks for waiting for my feedback despite my delays.