Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Dec 14, 2020

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

@legendecas legendecas added the node-api Issues and PRs related to the Node-API. label Dec 14, 2020
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Dec 14, 2020
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@gabrielschulhof
Copy link
Contributor

@mhdawson is this less than semver-major?

@mhdawson
Copy link
Member

mhdawson commented Jan 4, 2021

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

@legendecas legendecas marked this pull request as ready for review January 14, 2021 17:35
@legendecas legendecas force-pushed the napi-tsfn branch 2 times, most recently from 91711df to f156f87 Compare January 14, 2021 17:46
@legendecas legendecas changed the title n-api: emit uncaught-exception on unhandled tsfn callbacks n-api: emit uncaught-exception on calling into modules Jan 14, 2021
@aduh95
Copy link
Contributor

aduh95 commented Jan 18, 2021

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

@mhdawson
Copy link
Member

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 mhdawson added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jan 18, 2021
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,

Comment on lines 116 to 123
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);
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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); });

?

@@ -33,6 +33,8 @@ using Persistent = v8::Global<T>;

using PersistentToLocal = node::PersistentToLocal;

void trigger_fatal_exception(napi_env env, v8::Local<v8::Value> local_err);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src/node_api.cc Outdated
Comment on lines 51 to 76
void trigger_fatal_exception(napi_env env, v8::Local<v8::Value> local_err) {
v8::Local<v8::Message> local_msg =
v8::Exception::CreateMessage(env->isolate, local_err);
node::errors::TriggerUncaughtException(env->isolate, local_err, local_msg);
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

@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
Copy link
Contributor

aduh95 commented Jan 22, 2021

@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.

doc/api/cli.md Outdated
@@ -585,6 +585,13 @@ added: v9.0.0
Disables runtime checks for `async_hooks`. These will still be enabled
dynamically when `async_hooks` is enabled.

### `--no-force-napi-uncaught-exceptions-policy`
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@mhdawson mhdawson Jan 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link

ekraihan commented Feb 8, 2021

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
Copy link

ekraihan commented Feb 8, 2021

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.

@legendecas legendecas force-pushed the napi-tsfn branch 2 times, most recently from e679ba5 to b900bd6 Compare March 4, 2021 16:30
juanarbol pushed a commit that referenced this pull request Oct 4, 2022
Deprecation should reference a valid deprecation code.

PR-URL: #44624
Refs: #36510
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
juanarbol pushed a commit that referenced this pull request Oct 4, 2022
Deprecation should reference a valid deprecation code.

PR-URL: #44624
Refs: #36510
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
juanarbol pushed a commit that referenced this pull request Oct 4, 2022
Deprecation should reference a valid deprecation code.

PR-URL: #44624
Refs: #36510
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
juanarbol pushed a commit that referenced this pull request Oct 4, 2022
Deprecation should reference a valid deprecation code.

PR-URL: #44624
Refs: #36510
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
juanarbol pushed a commit that referenced this pull request Oct 7, 2022
Deprecation should reference a valid deprecation code.

PR-URL: #44624
Refs: #36510
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
Notable changes:

Adds `util.parseArgs` helper for higher level command-line argument
parsing.
Contributed by Benjamin Coe, John Gee, Darcy Clarke, Joe Sepi,
Kevin Gibbons, Aaron Casanova, Jessica Nahulan, and Jordan Harband.
nodejs/node#42675

Node.js ESM Loader hooks now support multiple custom loaders, and
composition is achieved via "chaining": `foo-loader` calls `bar-loader`
calls `qux-loader` (a custom loader _must_ now signal a short circuit
when intentionally not calling the next). See the ESM docs
(https://nodejs.org/dist/latest-v16.x/docs/api/esm.html) for details.
Contributed by Jacob Smith, Geoffrey Booth, and Bradley Farias.
nodejs/node#42623

The `node:test` module, which was initially introduced in Node.js
v18.0.0, is now available with all the changes done to it up to Node.js
v18.7.0.

To better align Node.js' experimental implementation of the Web Crypto
API with other runtimes, several changes were made:
* Support for CFRG curves was added, with the `'Ed25519'`, `'Ed448'`,
  `'X25519'`, and `'X448'` algorithms.
* The proprietary `'NODE-DSA'`, `'NODE-DH'`, `'NODE-SCRYPT'`,
  `'NODE-ED25519'`, `'NODE-ED448'`, `'NODE-X25519'`, and `'NODE-X448'`
  algorithms were removed.
* The proprietary `'node.keyObject'` import/export format was removed.
Contributed by Filip Skokan.
nodejs/node#42507
nodejs/node#43310

Updated Corepack to 0.12.1 - nodejs/node#43965
Updated ICU to 71.1 - nodejs/node#42655
Updated npm to 8.15.0 - nodejs/node#43917
Updated Undici to 5.8.0 - nodejs/node#43886

(SEMVER-MINOR) crypto: make authTagLength optional for CC20P1305 (Tobias Nießen) nodejs/node#42427
(SEMVER-MINOR) crypto: align webcrypto RSA key import/export with other implementations (Filip Skokan) nodejs/node#42816
(SEMVER-MINOR) dns: export error code constants from `dns/promises` (Feng Yu) nodejs/node#43176
doc: deprecate coercion to integer in process.exit (Daeyeon Jeong) nodejs/node#43738
(SEMVER-MINOR) doc: deprecate diagnostics_channel object subscribe method (Stephen Belanger) nodejs/node#42714
(SEMVER-MINOR) errors: add support for cause in aborterror (James M Snell) nodejs/node#41008
(SEMVER-MINOR) events: expose CustomEvent on global with CLI flag (Daeyeon Jeong) nodejs/node#43885
(SEMVER-MINOR) events: add `CustomEvent` (Daeyeon Jeong) nodejs/node#43514
(SEMVER-MINOR) events: propagate abortsignal reason in new AbortError ctor in events (James M Snell) nodejs/node#41008
(SEMVER-MINOR) fs: propagate abortsignal reason in new AbortSignal constructors (James M Snell) nodejs/node#41008
(SEMVER-MINOR) fs: make params in writing methods optional (LiviaMedeiros) nodejs/node#42601
(SEMVER-MINOR) fs: add `read(buffer[, options])` versions (LiviaMedeiros) nodejs/node#42768
(SEMVER-MINOR) http: add drop request event for http server (theanarkh) nodejs/node#43806
(SEMVER-MINOR) http: add diagnostics channel for http client (theanarkh) nodejs/node#43580
(SEMVER-MINOR) http: add perf_hooks detail for http request and client (theanarkh) nodejs/node#43361
(SEMVER-MINOR) http: add uniqueHeaders option to request and createServer (Paolo Insogna) nodejs/node#41397
(SEMVER-MINOR) http2: propagate abortsignal reason in new AbortError constructor (James M Snell) nodejs/node#41008
(SEMVER-MINOR) http2: compat support for array headers (OneNail) nodejs/node#42901
(SEMVER-MINOR) lib: propagate abortsignal reason in new AbortError constructor in blob (James M Snell) nodejs/node#41008
(SEMVER-MINOR) lib: add abortSignal.throwIfAborted() (James M Snell) nodejs/node#40951
(SEMVER-MINOR) lib: improved diagnostics_channel subscribe/unsubscribe (Stephen Belanger) nodejs/node#42714
(SEMVER-MINOR) module: add isBuiltIn method (hemanth.hm) nodejs/node#43396
(SEMVER-MINOR) module,repl: support 'node:'-only core modules (Colin Ihrig) nodejs/node#42325
(SEMVER-MINOR) net: add drop event for net server (theanarkh) nodejs/node#43582
(SEMVER-MINOR) net: add ability to reset a tcp socket (pupilTong) nodejs/node#43112
(SEMVER-MINOR) node-api: emit uncaught-exception on unhandled tsfn callbacks (Chengzhong Wu) nodejs/node#36510
(SEMVER-MINOR) perf_hooks: add PerformanceResourceTiming (RafaelGSS) nodejs/node#42725
(SEMVER-MINOR) report: add more heap infos in process report (theanarkh) nodejs/node#43116
(SEMVER-MINOR) src: add --openssl-legacy-provider option (Daniel Bevenius) nodejs/node#40478
(SEMVER-MINOR) src: define fs.constants.S_IWUSR & S_IRUSR for Win (Liviu Ionescu) nodejs/node#42757
(SEMVER-MINOR) src,doc,test: add --openssl-shared-config option (Daniel Bevenius) nodejs/node#43124
(SEMVER-MINOR) stream: use cause options in AbortError constructors (James M Snell) nodejs/node#41008
(SEMVER-MINOR) stream: add iterator helper find (Nitzan Uziely) nodejs/node#41849
(SEMVER-MINOR) stream: add writableAborted (Robert Nagy) nodejs/node#40802
(SEMVER-MINOR) timers: propagate signal.reason in awaitable timers (James M Snell) nodejs/node#41008
(SEMVER-MINOR) v8: add v8.startupSnapshot utils (Joyee Cheung) nodejs/node#43329
(SEMVER-MINOR) v8: export more fields in getHeapStatistics (theanarkh) nodejs/node#42784
(SEMVER-MINOR) worker: add hasRef() to MessagePort (Darshan Sen) nodejs/node#42849

PR-URL: nodejs/node#44098
juanarbol pushed a commit that referenced this pull request Oct 10, 2022
Deprecation should reference a valid deprecation code.

PR-URL: #44624
Refs: #36510
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
juanarbol pushed a commit that referenced this pull request Oct 11, 2022
Deprecation should reference a valid deprecation code.

PR-URL: #44624
Refs: #36510
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this pull request Jan 3, 2023
Deprecation should reference a valid deprecation code.

PR-URL: nodejs/node#44624
Refs: nodejs/node#36510
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this pull request Jan 3, 2023
Deprecation should reference a valid deprecation code.

PR-URL: nodejs/node#44624
Refs: nodejs/node#36510
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
nodejs-github-bot pushed a commit that referenced this pull request Sep 25, 2023
This enables the option `--force-node-api-uncaught-exceptions-policy`
for a specific Node-API addon when it is compiled with
`NAPI_EXPERIMENTAL` (and this would be the default behavior when
`NAPI_VERSION` 10 releases). This would not break existing Node-API
addons.

PR-URL: #49313
Refs: #36510
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
ruyadorno pushed a commit that referenced this pull request Sep 28, 2023
This enables the option `--force-node-api-uncaught-exceptions-policy`
for a specific Node-API addon when it is compiled with
`NAPI_EXPERIMENTAL` (and this would be the default behavior when
`NAPI_VERSION` 10 releases). This would not break existing Node-API
addons.

PR-URL: #49313
Refs: #36510
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
ruyadorno pushed a commit that referenced this pull request Sep 28, 2023
This enables the option `--force-node-api-uncaught-exceptions-policy`
for a specific Node-API addon when it is compiled with
`NAPI_EXPERIMENTAL` (and this would be the default behavior when
`NAPI_VERSION` 10 releases). This would not break existing Node-API
addons.

PR-URL: #49313
Refs: #36510
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
This enables the option `--force-node-api-uncaught-exceptions-policy`
for a specific Node-API addon when it is compiled with
`NAPI_EXPERIMENTAL` (and this would be the default behavior when
`NAPI_VERSION` 10 releases). This would not break existing Node-API
addons.

PR-URL: nodejs#49313
Refs: nodejs#36510
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
targos pushed a commit that referenced this pull request Nov 27, 2023
This enables the option `--force-node-api-uncaught-exceptions-policy`
for a specific Node-API addon when it is compiled with
`NAPI_EXPERIMENTAL` (and this would be the default behavior when
`NAPI_VERSION` 10 releases). This would not break existing Node-API
addons.

PR-URL: #49313
Refs: #36510
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
This enables the option `--force-node-api-uncaught-exceptions-policy`
for a specific Node-API addon when it is compiled with
`NAPI_EXPERIMENTAL` (and this would be the default behavior when
`NAPI_VERSION` 10 releases). This would not break existing Node-API
addons.

PR-URL: nodejs/node#49313
Refs: nodejs/node#36510
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
This enables the option `--force-node-api-uncaught-exceptions-policy`
for a specific Node-API addon when it is compiled with
`NAPI_EXPERIMENTAL` (and this would be the default behavior when
`NAPI_VERSION` 10 releases). This would not break existing Node-API
addons.

PR-URL: nodejs/node#49313
Refs: nodejs/node#36510
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pass exceptions to JS that are thrown by JS functions in an async context