node-api: run Node-API finalizers in GC second pass

[vmoroz]

The issue

Currently Reference finalizers are run inside of SetImmediate.
In case if user code creates a lot of native objects in the main script, it could cause a significant memory pressure, even if the objects are properly released. This is because they are "collected" only inside of SetImmediate that follows the script run.
See issue: nodejs/node-addon-api#1140

In the a74a6e3 commit the processing of finalizers was moved from the GC second pass to the SetImmediate because finalizers may throw JavaScript exceptions which can affect behavior of other functions.
If the JavaScript exception is thrown inside of SetImmediate, then it causes an unhandled exception which can be handled process-wide with help of process.on('uncaughtException', ...) event handler.

The solution

In this PR we are switching back to processing finalizers in the GC second pass which may happen any time after GC calls.
To address the issue of handling JS exceptions from finalizers we do the following in this PR:

• By default, all JS exceptions from finalizers cause the Node.js unhandled exceptions. We apply the same error handling mechanism as used by immediate tasks created by SetImmediate. Thus, we address the previous issue with the finalizer JS errors interrupting other functions, and align the error handling behavior with SetImmediate.
• In addition to it, we are adding new node_api_set_finalizer_error_handler public API to setup an error handler per napi_env instance which is created per each native module. Each native module can handle its finalizer JS errors on its own.

Tests

New test_finalizer_exception test is added to js-native-api to verify the new behavior.
All other js-native-api, node-api, and cctests are passing.

Documentation

The n-api.md documentation is updated with the new node_api_set_finalizer_error_handler public API.
See the n-api.md for the details about the node_api_set_finalizer_error_handler function.

[nodejs-github-bot]

Review requested:

• @nodejs/node-api

[Trott]

@nodejs/node-api Does this require any documentation additions or new tests?

[vmoroz]

@nodejs/node-api Does this require any documentation additions or new tests?

It is changing the internal behavior - I am not sure about the docs.
Though, we definitely need a new test for it.
I hope to discuss this change with the Node-API team tomorrow and if the overall approach makes sense, then I will add the test and any required docs.

[vmoroz]

While I work on the unit test, I see that I was completely wrong about the root cause of the issue.
I thought that the issue was the run of the GC second pass callback, while the true issue is with the Node-API code that always runs finalizers as SetImmediate. See:

node/src/node_api.cc

Lines 36 to 48 in b35ee26

	void node_napi_env__::CallFinalizer(napi_finalize cb, void* data, void* hint) {
	// we need to keep the env live until the finalizer has been run
	// EnvRefHolder provides an exception safe wrapper to Ref and then
	// Unref once the lambda is freed
	EnvRefHolder liveEnv(static_cast<napi_env>(this));
	node_env()->SetImmediate(
	[=, liveEnv = std::move(liveEnv)](node::Environment* node_env) {
	napi_env env = liveEnv.env();
	v8::HandleScope handle_scope(env->isolate);
	v8::Context::Scope context_scope(env->context());
	env->CallIntoModule([&](napi_env env) { cb(env, data, hint); });
	});
	}

Converting the status of the PR to a draft for now.

[vmoroz]

Running all finalizers in the SetImmediate was introduced by this commit: a74a6e3

It sounds like that in order to do it right we must:

• call finalizers synchronously while draining the finalizing queue
• stop draining on the first JS exception
• change the draining of the queue from the GC second pass to still use the SetImmediate to conform the existing behavior
• have tests for both cases with and without exceptions

[vmoroz]

We had discussed this PR in our Node-API meeting today on 3/11/2022. The key takeaways from the discussion:

• It is a good idea to have a configurable feature that controls whether to drain finalizer queue from Node-API methods that may touch GC or not.
• Returning JS errors from finalizers as a part of other Node-API method calls does not seem to be right. We must follow the same error reporting pattern as we have today with the use of SetImmediate.
• The overall approach based on the finalizing queue seems to be promising.

Some thoughts about the finalizer error handling after the meeting:

• By default, JS errors in finalizers should be treated as unhandled JS exceptions as we do it today.
• We should add a new public method node_api_set_finalizer_error_handler that can handle finalizer errors per addon.
• The node_api_call_finalizers should return finalizer error or allow to choose it to be handled by local or global handlers discussed above.
• Document how JS errors should be handled inside of the finalizer code and what happens when they are not handled.

[vmoroz]

I ended up removing the finalizing_queue in favor of using GC second pass as it used to be before a74a6e3 and fixing the error handing mechanism for finalizers.

[vmoroz]

Sorry, I accidently removed the PR branch. The PR is re-created as #42515