ThreadSafeFunction should handle exceptions in async callbacks identical to Node itself

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.

Here is what I did:
mmomtchev/exprtk.js@2d0d34e

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

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.

I believe this is addressed by the core pr: nodejs/node#36510

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

@mmomtchev

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

No, I am impacted by this throw in

node-addon-api/napi-inl.h

Line 2362 in 63d3c30

NAPI_RETURN_OR_THROW_IF_FAILED(

which is not covered

@mhdawson

@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

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.

@mmomtchev

@mmomtchev right now the new behavior should be off by default see - nodejs/node#36510 (comment). 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?

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

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.

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

@legendecas

@legendecas will look at:

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

I believe nodejs/node#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 nodejs/node#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 nodejs/node#42557 (comment).

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.

Navigation Menu

Repository navigation

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

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

ThreadSafeFunction should handle exceptions in async callbacks identical to Node itself #1119

Description

Activity

mmomtchev commented on Jan 2, 2022

github-actions commented on Apr 3, 2022

KevinEady commented on May 3, 2022

mhdawson commented on May 6, 2022

mhdawson commented on Jun 3, 2022

mmomtchev commented on Jun 3, 2022

mmomtchev commented on Jun 4, 2022

mmomtchev commented on Jun 4, 2022

mhdawson commented on Jun 13, 2022

mmomtchev commented on Jun 13, 2022

github-actions commented on Sep 12, 2022

mhdawson commented on Sep 23, 2022

mhdawson commented on Sep 23, 2022

legendecas commented on Oct 17, 2022

github-actions commented on Jan 16, 2023

github-actions commented on Apr 28, 2023

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

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