napi_make_callback() with null async_context doesn't reuse current context

[Flarna]

• Version: 14.10.1, 12.18.3, 10.22.0
• Platform: all
• Subsystem: napi, async_hooks

What steps will reproduce the bug?

Call back sync to javascript from native using napi_make_callback() with async_context set to NULL.
The simplest way to reproduce this is to add following code in test/node-api/test_make_callback_recurse/test.js

const { executionAsyncId } = require("async_hooks")
const eId = executionAsyncId();
makeCallback("foo", common.mustCall(function() {
  assert.strictEqual(executionAsyncId(), eId); // executionAsyncId is 0 here
}));

According to docs I would expect the test to pass as they tell However NULL is also allowed, which indicates the current async context (if any) is to be used for the callback..

How often does it reproduce? Is there a required condition?

Always

What is the expected behavior?

I would expect that the current async id is not changed.

What do you see instead?

async_id 0 is used

Additional information

I'm not sure here if the docs should be corrected or the implementation.
Maybe we should even disallow to pass NULL to napi_make_callback().

Relevant place in source:

node/src/node_api.cc

Lines 732 to 734 in 9d12c14

	if (node_async_context == nullptr) {
	static node::async_context empty_context = { 0, 0 };
	node_async_context = &empty_context;

Please note also that the c++ NAPI wrapper uses NULL as default argument for MakeCallback (see https://github.com/nodejs/node-addon-api/blob/6562e6b0ab604fd55b9c2d0cf954c9ce93e4bdee/napi.h#L1056-L1065).

[Flarna]

fyi @nodejs/n-api I'm happy to create a PR to correct this but I'm not sure if it is a doc issue or a implemenation issue.

[mhdawson]

We discussed in the meeting today, and we think that it's most likely a bug versus doc issue. PR would be welcome.