n-api: emit uncaught-exception on calling into modules

[legendecas]

Currently, those exceptions were swallowed and the execution may continue in an unstable state.

Following cases are covered in the PR that exceptions can be thrown on calling into modules:

1. TSFN callbacks
2. Finalizers

Fixes: #36402

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[mhdawson]

LGTM

[gabrielschulhof]

@mhdawson is this less than semver-major?

[mhdawson]

@gabrielschulhof that is a good question. It's clearly the "wrong" behaviour. Lets discuss in the next N-API team meeting

[aduh95]

Is this ready to land? What about semver major status?

[mhdawson]

We discussed this in the last node-api team meeting and discussed a few options but I don't think we've closed out the discussion yet. To summarize what we suggested might be a way forward:

• Land, mark as SemVer Major and don't backport to earlier Node.js versions. This ensures that any existing node-api addons will continue to work on older versions.
• Add a fallback command line option to revert to the old behavior, possibly something like --node-api-strict-exceptions=false. This will allow existing node-api addons to continue to work on newer versions if the change does cause issues
• Like fallback options added when we make a potentially breaking change in a SemVer minor security release, immediately deprecate the option and remove from the next Major.

The change does not affect the shape of the node-api so addons would continue to compile/run so its not breaking in that sense. However, it is a change in behavior such that addons that used to "appear" to run ok even though there were unhandled exceptions that reported/handled could end up failing with an uncaught exception error.

Alternatives would be to introduced new APIs in node-api to enable the strict mode, however, that would take an developer changes to get the right behavior and would likely stretch out the period where the wrong behavior is in place.

We hope/believe that the change should have limited impact on existing addons, but the approach suggested above seemed like a good way to balance the risk of moving to the correct behavior.

[mhdawson]

Changing to request changes as we'd want to add the command line option to fall back to the original behavior as part of the PR,

[gabrielschulhof]

Why not just

Suggested change

	CallIntoModule([&](napi_env env) { cb(env, data, hint); },
	[](napi_env env, v8::Local<v8::Value> local_err) {
	// If there was an unhandled exception in the complete
	// callback, report it as a fatal exception. (There is no
	// JavaScript on the callstack that can possibly handle
	// it.)
	v8impl::trigger_fatal_exception(env, local_err);
	});
	CallbackIntoModule([&](napi_env env) { cb(env, data, hint); });

?

[gabrielschulhof]

It might be better to make this a virtual member function of struct napi_env__ which ... does nothing? ... and override it in struct node_napi_env__. That way there is a default implementation for those who do not include the rest of Node.js.

[gabrielschulhof]

Could you maybe move this back to where it was and add back the static inline? It would reduce the size of the diff.

[gabrielschulhof]

@legendecas if this fixes #36402 can you please add that to the commit message? That way, when this lands, that issue will be closed.

[aduh95]

@legendecas if this fixes #36402 can you please add that to the commit message? That way, when this lands, that issue will be closed.

FWIW ncu/git node land does add this kind of metadata to the commit message on landing if it detects it in the PR OP.

[legendecas]

We can name it with node-api instead of napi per nodejs/abi-stable-node#420

[mhdawson]

I was also thinking it might be something like

--force-node-api-uncaught-exceptions-policy=false

with the default being the equivalent to

--force-node-api-uncaught-exceptions-policy=true

If we find it ends up causing too much breakage then we'd change the default to false..

[ekraihan]

Something possibly related to this code review: if a JS function marked async is called from C++ and then that JS function throws, the usual UnhandledPromiseRejectionWarning is NOT raised. Is there a way to make sure that warning is raised?

[ekraihan]

Something possibly related to this code review: if a JS function marked async is called from C++ and then that JS function throws, the usual UnhandledPromiseRejectionWarning is NOT raised. Is there a way to make sure that warning is raised?

I'm sorry, this was wrong. I was running my code under the mocha test framework which seems to change unhandled-promise-rejection behavior in the background.