Integrate C++ AsyncHooks Embedder API with native abstraction

[AndreasMadsen]

• Version: v8.x
• Platform: all
• Subsystem: n-api, nan, async_hooks

The AsyncHooks Embedder API has now been merged, we need to integrate this into N-API and NAN such that userland add-ons can inform async_hooks about the context.

I'm not very familiar with either APIs, but NAN is the API I know the best, so I will explain it from that perspective.

AsyncHooks allows userland to get notified about all asynchronous event and understand what caused the asynchronous job to be tasked. This requires 4 events to be emitted:

• init: emitted with the asynchronous job is created (called a resource). EmitAsyncInit emits this.
• before, after: emitted with the asynchronous job calls back, this can happen multiple times. MakeCallback now emits these when two additional parameters are passed (async_id and trigger_id).
• destroy: emitted when the resource can't call back anymore. EmitAsyncDestroy emits this.

there is also a high-level API, a C++ class called AsyncResource but I suspect this isn't useful for NAN or N-API.

In terms of NAN I think there is almost a 1 to 1 mapping between Nan::Callback and the AsyncHooks API. I believe the following changes should be made:

• Callback::Callback should call trigger_id = AsyncHooksGetTriggerId(isolate); and uid = EmitAsyncInit(isolate, resource, name, trigger_id);.
• Callback::Call should call node::MakeCallback(isolate, resource, callback, argc, argv, uid, trigger_id);
• Callback::~Callback should call EmitAsyncDestroy(isolate, uid);

This is very similar to the AsyncResource class. It is not clear what the resource should be as Callback::Callback does not take such a parameter.

I believe @mhdawson said during a diagnostics meeting that if NAN required changes then likely N-API would need changes too.

/cc @mhdawson @addaleax @trevnorris @nodejs/diagnostics @nodejs/n-api @nodejs/addon-api @nodejs/nan (@kkoopa)

[addaleax]

I can’t really speak for NAN; my best guess is that it will want to build its own thing similar to AsyncResource.

As for N-API: I’m not sure. We could just proxy the low-level API to C and add a napi_make_callback variant that takes id and trigger_id parameters, but that might require committing to the native async_hooks API as it currently is, i.e. make that non-experimental?

One thought I’ve had is that we could add a CallbackScope class that basically does what MakeCallback does but without necessarily jumping into JS; N-API could use that to make async worker’s complete callbacks automagically run in the correct async context. That wouldn’t require any API changes, too.

[kkoopa]

I need to familiarize myself more with this, but initially I think @addaleax is right in that it would be better to make a new class for this (new) concept. Nan::Callback has always been a convenience class which is mostly around for legacy reasons (legacy within NAN). It was useful in the Node 0.10 -> 0.12 transition with the Persistent changes to reduce leaky and broken code. However, making it do something new seems like it could backfire in unforseen ways.

Another question is that of compatibility. How far back can this be implemented without patching node.js?
All the way down to 0.10? NAN only offers functionality that works on all supported versions, but supports pick-and-mix with raw V8, libuv and anything else. That way one can write the bulk of code using NAN's abstractions and use, say, the V8 API for Futures if it will only run on versions of node that supports V8 Futures.

[AndreasMadsen]

However, making it do something new seems like it could backfire in unforseen ways.

I'm not exactly sure what "new" covers, I see this as adding annotation to existing functionality. My thoughts behind annotating Nan::Callback is that it would make the userland adoption transparent, but if very few are using Nan::Callback then that will of cause not be the case.

Adoption by userland is critical in this case, because if just one native addon model is missing the annotation then the usefulness of AsyncHooks is dramatically decreased.

Another question is that of compatibility. How far back can this be implemented without patching node.js?

Currently, AsyncHooks will only be in node v8.x, we may backport it to v6.x and perhaps even v4.x. As said the AsyncHooks Embedder API should be seen as annotation, thus for node versions where the AsyncHooks Embedder API is not supported that annotation simply won't be added.

[addaleax]

Adoption by userland is critical in this case, because if just one native addon model is missing the annotation then the usefulness of AsyncHooks is dramatically decreased.

I agree – once async_hooks is no longer experimental, we should probably deprecate the old functionality.

we may backport it to v6.x and perhaps even v4.x

I’m pretty sure we won’t backport to v4.x?

[kkoopa]

I do not know what the explicit usage rate of Nan::Callback is. It is however implicitly used in Nan::AsyncWorker, which most addons use for asynchronous functions. Holdouts are mostly projects that have been around for a long time and simply updated their existing, working baton code at some point.

I am not sure what "new" covers either, which is why I need to read up on what async hooks are supposed to do before I can make an informed statement.

However, the point of NAN is to offer a set of functionality that gives the same observable behavior on all supported versions of Node.js. There should not be anything that only works on specific versions, since that would defeat the point of having a version-independence abstraction layer. An end-user should be able to assume that all code does what it should on all supported versions. This also includes not having functionality that becomes a no-op on certain versions, since that hides that the code does not work as intended.

That being said, if all this does is adding some extra information which can be useful in some cases, there might be a way to add some of it to NAN so that code running on newer versions of node.js does something in a better or more efficient manner. For instance, if async hooks subsumes an older API which may then be deprecated, the NAN code for all newer versions that support this functionality could be made to use it instead of the older functionality, since that would fall within implementation, not version-independent functionality.

To make this a bit clearer: We can think of two aspects of NAN: One is the external, documented API. Only functionality which works on all supported versions goes here. The other is the internal implementation (actually there are several of them depending on node version), which has greater freedom. Here, the only restriction is: Given a program in state S which calls a part of NAN, at return from NAN, the program is in state S', regardless of which version-dependent implementation was used.
This is a truth with modification, but we try to keep it as close as possible. In practice, we may end up in a state S'' which is a superset of S'. While not ideal, this is sometimes the only practical way. What is not allowed is to sometimes end up in a state S''' which is a strict subset of S'.

[addaleax]

It is however implicitly used in Nan::AsyncWorker, which most addons use for asynchronous functions.

I think it should be easy enough to add support for everything we need to AsyncWorker transparently, because N-API is basically going to need the same things anyway.

There should not be anything that only works on specific versions, since that would defeat the point of having a version-independence abstraction layer.

You mean, that works from the addon writer’s perspective, right? It would be okay if Nan had code that only worked well with the versions of Node to which async_hooks has been ported, correct?

Also: Would it help if Node had a #define that indicated the presence of the C++ embedder API?

[kkoopa]

Yes, from the addon writer's perspective, if a (documented) function appears in NAN, the contract states that it works the same on all versions of Node.js. Suppose a function in NAN returns an object with a set of keys and values. Those keys and values should be the same on all versions of Node. In some specific instances it might be allowable to allow more keys and values to appear, but then they fall out of NAN's blessing, not documented by NAN and are used at your own risk (usually because we would not take active steps to remove kv-pairs if that returned object was obtained from, say, a V8 function).

Sniffing specific versions of Node from NODE_MODULE_VERSION and NODE_VERSION is not too hard, there are only ever problems when nonstandard implementations which are not Node but claim to be so enter the picture, and I suspect a special define could end up meaningless there too...

[kkoopa]

From what I now have gleaned about what the async hooks are supposed to do, it seems that all NAN needs to do is change implementations of relevant existing APIs for versions of Node that support async hooks so that async hooks transparently work, since the end user may want to use async hooks while running on newer versions of Node, while still using NAN. Since no new external APIs related to async hooks need to be added, that should be perfectly fine. It is a matter of transparency, or not getting in the way.

[jasongin]

@matthewloring and I had a discussion yesterday about exposing APIs for async hooks in N-API, and I've started looking into this further. Before proposing a design, I have a few questions:

• What is the reason for the async_context structure? It looks like it would be simpler for EmitAsyncInit() to just return the execution async ID, since the trigger ID is provided by the caller. Or could there be other kinds of async IDs that might be added to the async_context structure in the future? If the structure is likely to change, we should consider how such a change can be handled without breaking N-API's ABI stability.

• The async_id type is a typedef of double. Does that numeric value need to be exposed to users (maybe for logging/diagnostics) or can the type be completely opaque?

• Does N-API need to continue supporting domains? A possible N-API design for async hooks might involve only exposing the "primitive" methods for creating/getting async IDs and emitting async hooks events, without actually using the node::AsyncWrap class that currently includes domain integration. (The Napi::AsyncWorker class in node-addon-api would then help with calling the N-API async hooks C functions at the appropriate times.)

[trevnorris]

Does that numeric value need to be exposed to users (maybe for logging/diagnostics) or can the type be completely opaque?

It's only a double b/c that's the only numeric type that JS supports, and the async id is assigned as an object property for quick lookup. Can you explain what you mean by "opaque" more?

A possible N-API design for async hooks might involve only exposing the "primitive" methods for creating/getting async IDs and emitting async hooks events,

That API exists in in JS and would be possible in C++, but @AndreasMadsen is more knowledgeable as to whether there would be any issues doing this.

[addaleax]

It looks like it would be simpler for EmitAsyncInit() to just return the execution async ID, since the trigger ID is provided by the caller.

It is provided by default but the usual scenario is going to be that the trigger ID is inferred from context.

The async_id type is a typedef of double. Does that numeric value need to be exposed to users (maybe for logging/diagnostics) or can the type be completely opaque?

The typedef is a somewhat intentional step towards making it opaque, yes. It might still be good to be able to log the value, or to pass it to JS and have it comparable to the values JS sees.

Does N-API need to continue supporting domains?

Maybe/probably? It should not be hard, and while it would be really nice to have domains implementable in terms of async_hooks, we’re not there yet.

A possible N-API design for async hooks might involve only exposing the "primitive" methods for creating/getting async IDs and emitting async hooks events,

That API exists in in JS and would be possible in C++, but @AndreasMadsen is more knowledgeable as to whether there would be any issues doing this.

@trevnorris Not sure what you mean, the C++ API for this already exists.

Imo, the main thing that N-APi should focus on for now is enabling async_hooks and domains for the napi_async_work functions; that shouldn’t be hard and doesn’t even require exposing new native APIs.

[AndreasMadsen]

It looks like it would be simpler for EmitAsyncInit() to just return the execution async ID, since the trigger ID is provided by the caller.

@addaleax is right. I want to add that there is no API for getting the default value, other than EmitAsyncInit(). It also prevents some easy to make bugs. I think we would like to do the same in the JS Embedder API but unfortunately tubles/structs are too expensive in JS.

The async_id type is a typedef of double. Does that numeric value need to be exposed to users (maybe for logging/diagnostics) or can the type be completely opaque?

I have sometimes abused the fact that the double is increasing with time, which makes it an easy sortkey and thus I can use some clever graph hacks. But it should be opaque. Perhaps, at some point in the future we will use int64 instead.

A possible N-API design for async hooks might involve only exposing the "primitive" methods for creating/getting async IDs and emitting async hooks events,

That API exists in in JS and would be possible in C++, but @AndreasMadsen is more knowledgeable as to whether there would be any issues doing this.

I'm flattered, but I will mostly defer to @addaleax as she wrote that code. In the future I think we would like to rewrite domains to use async_hooks. But it is unclear right now if that would be too breaking for users. I don't think it will be too breaking but it is hard to tell for sure.

[addaleax]

In the future I think we would like to rewrite domains to use async_hooks. But it is unclear right now if that would be too breaking for users. I don't think it will be too breaking but it is hard to tell for sure.

By the way, I tried that a while back, and… the tricky bit isn’t implementing domains on top of async_hooks, the tricky bit is going to be that Node’s unhandled exception handling is a horrible mess of spaghetti code. We should completely re-write that, but that’s going to be a breaking change in itself.

[trevnorris]

@addaleax

Not sure what you mean, the C++ API for this already exists.

Nevermind me. Was thinking of something else. It's what I get for trying to respond to issues while in a meeting.

@AndreasMadsen

In the future I think we would like to rewrite domains to use async_hooks. But it is unclear right now if that would be too breaking for users. I don't think it will be too breaking but it is h

When AsyncListener was first introduced I spend a month trying to do this. Got it ~90% the way there, but do to some fundamental differences the last 10% was impossible.

My initial work on this is at nodejs/node-v0.x-archive@bc39bdd but after the month of effort getting the async error handling down correctly I decided to give up. Unfortunately the commits for those changes were lost when I deleted my joyent/node folder.

[jasongin]

It's only a double b/c that's the only numeric type that JS supports, and the async id is assigned as an object property for quick lookup.

I don't understand that reasoning. It could easily be treated as an integer in the native API. While working on performance optimizations for N-API, I found that creating a V8 number value is somewhat faster using v8::Integer::New(int32) compared to v8::Number::New(double), even though the latter will eventually still create an optimized integer representation when the double value has no fractional component.

Can you explain what you mean by "opaque" more?

A pointer to an undefined structure. N-API uses opaque pointers for some things so that the actual type or structure of a value is hidden from user code and can change without breaking ABI stability.

Imo, the main thing that N-APi should focus on for now is enabling async_hooks and domains for the napi_async_work functions; that shouldn't be hard and doesn't even require exposing new native APIs.

OK, that should be relatively easy to do. I still think eventually we will need to expose more of the async hooks APIs directly, but that can be lower priority.

[ofrobots]

First Nan PR: nodejs/nan#729

[ofrobots]

Deprecation of legacy node::MakeCallback node.h APIs: #18632

[ofrobots]

Nan::AsyncWorker and Nan::Callback support: nodejs/nan#731

[ofrobots]

With the nan changes landed, I believe this is ready to be closed now. Please reopen if anyone disagrees.

[mike-kaufman]

Thanks @ofrobots for driving this. :)