`ThreadSafeFunction` should handle exceptions in async callbacks identical to Node itself

[mmomtchev]

This is a duplicate of #669

When a JS callback called from a ThreadSafeFunction throws an exception, the result is:

terminate called after throwing an instance of 'Napi::Error'
  what():  unexpected error
Aborted (core dumped)

While this:

setImmediate(() => {
  throw new Error('unexpected error');
});

ends up like this:

/home/mmom/src/exprtk.js/uncaught.js:2
    throw new Error('err');
    ^

Error: err
    at Immediate.<anonymous> (/home/mmom/src/exprtk.js/uncaught.js:2:9)
    at processImmediate (internal/timers.js:464:21)

Why is it such a big deal? Because, as a module author, the first one will surely end up in an issue for my module, while with the second one, the user will correctly recognize what is happening.

Ideally, there should be a way to call Node's internal method which terminates the process.

[mmomtchev]

Here is what I did: https://github.com/mmomtchev/exprtk.js/commit/2d0d34e999124bc3602e2b679ed332e0163eb160

Ideally, I want to be able to call this: https://github.com/nodejs/node/blob/64cc066f59f50e29fc3a587a3cfc8b1270eb45cd/src/node_errors.cc#L856

[github-actions[bot]]

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.

[KevinEady]

I believe this is addressed by the core pr: https://github.com/nodejs/node/pull/36510

[mhdawson]

Discussed in the node-api team meeting and agreed it should be resolved by #36510

[mhdawson]

@mmomtchev can you check that #36510 which has landed in the primary branch resolved the issue for you

[mmomtchev]

No, I am impacted by this throw in https://github.com/nodejs/node-addon-api/blob/63d3c30ec12187cce72507a388487e4002cf6269/napi-inl.h#L2362 which is not covered

[mmomtchev]

@mhdawson This is a problem because after #36510 the only possible fix is to leave the exception handling in node-addon-api::Function::MakeCallback to Node itself - which means that without it exceptions will be silently ignored. This means that node-addon-api will have to detect if this PR is there or not - especially since its exception handling runs after node-addon-api::Function::MakeCallback

[mmomtchev]

In fact, I think that the fundamental problem is that #36510 is an API-breaking PR that requires a NAPI_VERSION bump - before this PR napi_make_callback left the exception handling to the user while now it handles it itself. I also wonder if there is (if there is, I can't see it) a way to for a module to force the ignoring of this exception. node-addon-api should use this method to force the ignoring of the exception on newer Node.js versions and then handle it itself - like it used to do on previous versions.

[mhdawson]

@mmomtchev right now the new behavior should be off by default see - https://github.com/nodejs/node/pull/36510#issuecomment-1050231172. Is that not the case?

We should be able to check the flag in node-api (I think) and use that to choose how to handle the exception.

It's possible we still need a chagne on the node-addon-api side to do this though. (ie if flag is set to do the right thing)

Am I missing something?

[mmomtchev]

If you can check for this flag while remaining backwards compatible, sure, this can be a solution.

Still, it will be somewhat problematic for the end user (the module author) - is it up to him to check for raised exceptions or not - as there will be two possibilities within the same version number. But since it is possible to easily handle both, it might stay like this. Currently a module that calls a ThreadSafeFunction must check for exceptions to not trigger the abort() which looks as a crash to the end user (the module user): https://github.com/mmomtchev/exprtk.js/blob/dbeb2d14e793e160ebac91355972330d9ed5b95a/src/async.h#L240

[github-actions[bot]]

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.

[mhdawson]

Next steps:

• try running reproducer with the command line flag added to core. Validate that it fixes the issue
• if fixed by runtime command line flag, look at introducing a feature flag that allows it to be turned on

[mhdawson]

@legendecas will look at:

try running reproducer with the command line flag added to core. Validate that it fixes the issue

[legendecas]

I believe https://github.com/nodejs/node/pull/36510 can handle the uncaught exceptions in async callbacks. However, as the flag requires the user to explicitly opt-in, the solution might not be optimal for library authors.

Library authors now can still emit an uncaughtException with https://nodejs.org/api/n-api.html#napi_fatal_exception. This can help library authors to migrate to the behavior enforced in https://github.com/nodejs/node/pull/36510.

As a follow-up, we'll investigate how to help add-ons to migrate to expected runtime behavior, without any breakages, like the flags that are being worked at https://github.com/nodejs/node/pull/42557#discussion_r995559115.

[github-actions[bot]]

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.

[github-actions[bot]]

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.