-
Notifications
You must be signed in to change notification settings - Fork 29k
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@mhdawson is this less than semver-major? |
@gabrielschulhof that is a good question. It's clearly the "wrong" behaviour. Lets discuss in the next N-API team meeting |
91711df
to
f156f87
Compare
Is this ready to land? What about semver major status? |
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:
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. |
There was a problem hiding this 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,
src/js_native_api_v8.h
Outdated
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); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just
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); }); |
?
src/js_native_api_v8_internals.h
Outdated
@@ -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); |
There was a problem hiding this comment.
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
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); | ||
} | ||
|
There was a problem hiding this comment.
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.
@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/ |
f9bbad8
to
ae41fe3
Compare
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` |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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..
Something possibly related to this code review: if a JS function marked |
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. |
e679ba5
to
b900bd6
Compare
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]>
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]>
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]>
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]>
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]>
PR-URL: nodejs/node#36510 Fixes: nodejs/node#36402 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
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
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]>
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]>
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]>
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]>
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]>
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]>
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]>
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]>
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]>
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]>
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]>
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:
Fixes: #36402
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesdocumentation is changed or added