node::MakeCallback(), napi_make_callback() requires recv to be an object / not empty

[simonbuchan]

• Version: v11.10.0
• Platform: Windows 10 64bit
• Subsystem:

napi_make_callback(), like napi_call_function(), takes a recv value to be used as this in JS. Unlike napi_call_function(), however, it validates that the value is of v8 type object, meaning that you can't pass not only NULL, but also explicit JS undefined, global or many other valid(ish) JS this values.

The immediate error on the N-API side looks like it was in from the start in ba7bac5 where a lot of null checking was added: note that napi_call_function() gets lots of checks added, but recv is directly passed through to v8.

I thought this was a bit strange, so I dug in a bit further to see if it was a requirement for node::MakeCallback, and it seems since #14697 the underlying node::MakeCallback() and node::CallbackScope() now also requires recv to not be empty with CHECK(!recv.IsEmpty()) on the line https://github.com/nodejs/node/pull/14697/files#diff-cd53544f44aab2c697bcd7b6a57f23ccR1398 despite the previous code taking care to handle it being null (when not using domains, at least), and the new code not requiring the value as far as I could tell?

Am I missing something obvious?

[bnoordhuis]

node::MakeCallback() requiring an object as the receiver has been true ever since its inception because it predates strict mode.

napi_make_callback() is a thin wrapper around node::MakeCallback() and therefore inherits that restriction.

That restriction cannot be lifted unconditionally because e.g. domain callbacks (as in: require('domain').create()) are closely tied to node::MakeCallback() and expect that the receiver is an object.

If we end up with something where it's technically possible to omit the receiver but practically speaking it's not, then we might as well leave well enough alone.

[simonbuchan]

Thanks for the reply! The workaround I'm using now is (trimmed for readability):

napi_value context_object, recv;
napi_create_object(env, &context_object);
napi_get_undefined(env, &recv);

napi_open_callback_scope(env, context_object, async_context, &callback_scope);

napi_call_function(env, recv, func, ...);

Which seems to work fine, as the context object is decoupled from the receiver. I don't really understand why the context object exists, or how domains affect this, is this dumb and wrong?

[addaleax]

@simonbuchan context_object should ideally match what was passed to napi_async_init.

The tl;dr is: It’s not currently used in napi_open_callback_scope. But a number of people think that the async_hooks API should be changed to not use integer IDs to refer to async resources, and that the resource objects themselves should be used for identifying asynchronous resources. In that case we’d need the napi_open_callback_scope() argument.

See e.g. #21313 for a previous attempt at moving into this direction – I don’t know if @mcollina is still interested in changing the async_hooks API to address this?

[simonbuchan]

Hmm, If I understand what you mean, @addaleax this would mean the async_hooks callbacks could later look something like { init(...), before(id, resource), after(id, resource), destroy(id, resource) } (note extra resource args) and as napi_async_init()/_destroy() are tied to the init and destroy events, napi_open/close_callback_scope() are tied to before and after events, and thus the context_object values should match so a async_hooks client can compare resource instances instead of ids?

If that's intended, I would have expected node to be stashing the context_object in the napi_async_context so you can't pass the "wrong" value later, but maybe it's just some wiggle room for you guys.

I don't currently have a meaningful "context" instance, but it seems a simple napi_create_object() is good enough for identity, and maybe I can use something more meaningful later (in my usage, though, when "init" and "destroy" should happen is a bit ambiguous...)

[mcollina]

See e.g. #21313 for a previous attempt at moving into this direction – I don’t know if @mcollina is still interested in changing the async_hooks API to address this?

Let's what the diagnostics summit.

cc @nodejs/diagnostics

[jasnell]

ping @nodejs/n-api @nodejs/addon-api ... is there anything actionable here?

[mhdawson]

I don't think any changes are planned.

[mhdawson]

I'm closing this since there has been no follow up questions since my comment over a year ago. Please let us know if you think that was not the right thing to do.