node-api: segregate nogc APIs from rest via type system #50060

gabrielschulhof · 2023-10-06T06:43:48Z

We define a new type called node_api_nogc_env as the const version of napi_env and node_api_nogc_finalize as a variant of napi_finalize that accepts a node_api_nogc_env as its first argument.

We then modify those APIs which do not affect GC state as accepting a node_api_nogc_env. APIs accepting finalizer callbacks are modified to accept node_api_nogc_finalize callbacks. Thus, the only way to attach a napi_finalize callback, wherein Node-APIs affecting GC state may be called is to call node_api_post_finalizer from a node_api_nogc_finalize callback.

In keeping with the process of introducing new Node-APIs, this feature is guarded by NAPI_EXPERIMENTAL. Since this feature modifies APIs already marked as stable, it is additionally guared by NODE_API_EXPERIMENTAL_NOGC_ENV, so as to provide a further buffer to adoption. Nevertheless, both guards must be removed upon releasing a new version of Node-API.

nodejs-github-bot · 2023-10-06T06:43:53Z

Review requested:

@nodejs/gyp
@nodejs/node-api

bnoordhuis · 2023-10-06T08:52:06Z

I've read your comment and looked at the code but I don't really get it. As in: I understand the what but not the why.

gabrielschulhof · 2023-10-06T15:44:06Z

@bnoordhuis this setup would serve as a compile-time guard against calling APIs that would crash if called during the now-synchronous finalizer, because they write to an area owned exclusively by the GC during its cycle. It affects add-on maintainer workflow as follows:

With this change:

Maintainer builds their add-on.
Maintainer sees warnings/errors about passing incorrect pointer type into an API.
Maintainer reads docs and employs node_api_post_finalizer to defer those calls.
Maintainer builds their add-on.
Add-on works correctly.

Without this change:

Maintainer builds their add-on.
Maintainer runs unit tests which presumably exercise all add-on functionality, including cleanup during gc.
During GC, a finalizer is called which makes a call to an API which now crashes because of the new gc access check with a useful message indicating that some parts of the finalizer need to be deferred with node_api_post_finalizer.

Note that whether or not the add-on runs into the gc access check depends on how thoroughly the add-on maintainer tests the add-on. In a worse scenario, it is the add-on's user that runs into this check during normal operations.

bnoordhuis · 2023-10-06T16:59:53Z

Right, okay. Sounds sensible but:

the documentation should probably explain that?
this isn't "pure" in the no-side-effects sense most programmers understand that word. Maybe s/pure/nogc/g?

gabrielschulhof · 2023-10-06T17:15:00Z

@bnoordhuis absolutely! The documentation is definitely missing. I was planning on adding that. Being more specific rather than just "pure" is a good idea. We've been using the term while discussing #42651 and so it kinda stuck. Will rename.

gabrielschulhof · 2023-10-06T19:28:26Z

TODOs:

Test that the finalizer for napi_set_instance_data need not be nogc, because, although one cannot execute JS when the env is getting torn down, it's not because there's a GC cycle in progress. APIs will throw napi_pending_excetion or napi_cannot_run_js, but will not crash. Similarly to node_api_post_finalizer, napi_set_instance_data accepts a nogc env because it should be callable from either a sync finalizer or during normal operations. It also accepts a napi_finalizer, because, when called, the finalizer will not be running synchronously during a GC cycle.

test/js-native-api/test_finalizer/test_finalizer.c

src/js_native_api.h

src/node_api.h

src/js_native_api_types.h

vmoroz · 2023-11-10T15:57:59Z

src/js_native_api_types.h

@@ -115,6 +142,13 @@ typedef napi_value(NAPI_CDECL* napi_callback)(napi_env env,
 typedef void(NAPI_CDECL* napi_finalize)(napi_env env,
                                        void* finalize_data,
                                        void* finalize_hint);
+#if defined(NAPI_EXPERIMENTAL) && defined(NODE_API_EXPERIMENTAL_NOGC_ENV)
+typedef void(NAPI_CDECL* node_api_nogc_finalize)(node_api_nogc_env env,


Can we keep a single finalize callback? Are there situations where we must use one vs another?

As we discussed in today's meeting, we need the second type of callback in order to pass functions of that type to node_api_post_finalizer, because a callback passed to this new API must be identical to a regular napi_finalizer, but must have access to all Node-APIs.

vmoroz · 2023-11-10T16:00:43Z

src/js_native_api_v8.cc

@@ -38,6 +38,17 @@
 #define CHECK_NEW_FROM_UTF8(env, result, str)                                  \
  CHECK_NEW_FROM_UTF8_LEN((env), (result), (str), NAPI_AUTO_LENGTH)

+#define CHECK_NEW_STRING_ARGS(env, str, length, result)                        \


The change is already too big. I wonder if we can split up small improvements like this into a separate PR?
The same is about some changes in the docs.

gabrielschulhof · 2023-11-13T16:36:50Z

@vmoroz, @legendecas, and @mhdawson I rebased the PR after landing #50664. Please take another look!

mhdawson

LGTM

nodejs-github-bot · 2023-12-18T18:55:47Z

CI: https://ci.nodejs.org/job/node-test-pull-request/56369/

nodejs-github-bot · 2023-12-19T07:37:17Z

Landed in 7a216d5

We define a new type called `node_api_nogc_env` as the `const` version of `napi_env` and `node_api_nogc_finalize` as a variant of `napi_finalize` that accepts a `node_api_nogc_env` as its first argument. We then modify those APIs which do not affect GC state as accepting a `node_api_nogc_env`. APIs accepting finalizer callbacks are modified to accept `node_api_nogc_finalize` callbacks. Thus, the only way to attach a `napi_finalize` callback, wherein Node-APIs affecting GC state may be called is to call `node_api_post_finalizer` from a `node_api_nogc_finalize` callback. In keeping with the process of introducing new Node-APIs, this feature is guarded by `NAPI_EXPERIMENTAL`. Since this feature modifies APIs already marked as stable, it is additionally guared by `NODE_API_EXPERIMENTAL_NOGC_ENV`, so as to provide a further buffer to adoption. Nevertheless, both guards must be removed upon releasing a new version of Node-API. PR-URL: #50060 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Vladimir Morozov <vmorozov@microsoft.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

We define a new type called `node_api_nogc_env` as the `const` version of `napi_env` and `node_api_nogc_finalize` as a variant of `napi_finalize` that accepts a `node_api_nogc_env` as its first argument. We then modify those APIs which do not affect GC state as accepting a `node_api_nogc_env`. APIs accepting finalizer callbacks are modified to accept `node_api_nogc_finalize` callbacks. Thus, the only way to attach a `napi_finalize` callback, wherein Node-APIs affecting GC state may be called is to call `node_api_post_finalizer` from a `node_api_nogc_finalize` callback. In keeping with the process of introducing new Node-APIs, this feature is guarded by `NAPI_EXPERIMENTAL`. Since this feature modifies APIs already marked as stable, it is additionally guared by `NODE_API_EXPERIMENTAL_NOGC_ENV`, so as to provide a further buffer to adoption. Nevertheless, both guards must be removed upon releasing a new version of Node-API. PR-URL: nodejs#50060 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Vladimir Morozov <vmorozov@microsoft.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Backport-PR-URL: nodejs#51804 (cherry picked from commit 7a216d5)

We define a new type called `node_api_nogc_env` as the `const` version of `napi_env` and `node_api_nogc_finalize` as a variant of `napi_finalize` that accepts a `node_api_nogc_env` as its first argument. We then modify those APIs which do not affect GC state as accepting a `node_api_nogc_env`. APIs accepting finalizer callbacks are modified to accept `node_api_nogc_finalize` callbacks. Thus, the only way to attach a `napi_finalize` callback, wherein Node-APIs affecting GC state may be called is to call `node_api_post_finalizer` from a `node_api_nogc_finalize` callback. In keeping with the process of introducing new Node-APIs, this feature is guarded by `NAPI_EXPERIMENTAL`. Since this feature modifies APIs already marked as stable, it is additionally guared by `NODE_API_EXPERIMENTAL_NOGC_ENV`, so as to provide a further buffer to adoption. Nevertheless, both guards must be removed upon releasing a new version of Node-API. PR-URL: #50060 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Vladimir Morozov <vmorozov@microsoft.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Backport-PR-URL: #51804 (cherry picked from commit 7a216d5)

We define a new type called `node_api_nogc_env` as the `const` version of `napi_env` and `node_api_nogc_finalize` as a variant of `napi_finalize` that accepts a `node_api_nogc_env` as its first argument. We then modify those APIs which do not affect GC state as accepting a `node_api_nogc_env`. APIs accepting finalizer callbacks are modified to accept `node_api_nogc_finalize` callbacks. Thus, the only way to attach a `napi_finalize` callback, wherein Node-APIs affecting GC state may be called is to call `node_api_post_finalizer` from a `node_api_nogc_finalize` callback. In keeping with the process of introducing new Node-APIs, this feature is guarded by `NAPI_EXPERIMENTAL`. Since this feature modifies APIs already marked as stable, it is additionally guared by `NODE_API_EXPERIMENTAL_NOGC_ENV`, so as to provide a further buffer to adoption. Nevertheless, both guards must be removed upon releasing a new version of Node-API. PR-URL: #50060 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Vladimir Morozov <vmorozov@microsoft.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

This PR contains the following updates: | Package | Update | Change | |---|---|---| | [node](https://nodejs.org) ([source](https://github.com/nodejs/node)) | minor | `18.19.1` -> `18.20.0` | --- ### Release Notes <details> <summary>nodejs/node (node)</summary> ### [`v18.20.0`](https://github.com/nodejs/node/releases/tag/v18.20.0): 2024-03-26, Version 18.20.0 'Hydrogen' (LTS), @richardlau [Compare Source](nodejs/node@v18.19.1...v18.20.0) ##### Notable Changes ##### Added support for import attributes Support has been added for import attributes, to replace the old import assertions syntax. This will aid migration by making the new syntax available across all currently supported Node.js release lines. This adds the `with` keyword which should be used in place of the previous `assert` keyword, which will be removed in a future semver-major Node.js release. For example, ```console import "foo" assert { ... } ``` should be replaced with ```console import "foo" with { ... } ``` For more details, see - [#50134](nodejs/node#50134) - [#51622](nodejs/node#51622) Contributed by Nicolò Ribaudo in [#51136](nodejs/node#51136) and Antoine du Hamel in [#50140](nodejs/node#50140). ##### Doc deprecation for `dirent.path` Please use newly added `dirent.parentPath` instead. Contributed by Antoine du Hamel in [#50976](nodejs/node#50976) and [#51020](nodejs/node#51020). ##### Experimental node-api feature flags Introduces an experimental feature to segregate finalizers that affect GC state. A new type called `node_api_nogc_env` has been introduced as the const version of `napi_env` and `node_api_nogc_finalize` as a variant of `napi_finalize` that accepts a `node_api_nogc_env` as its first argument. This feature can be turned off by defining `NODE_API_EXPERIMENTAL_NOGC_ENV_OPT_OUT`. Contributed by Gabriel Schulhof in [#50060](nodejs/node#50060). ##### Root certificates updated to NSS 3.98 Certificates added: - Telekom Security TLS ECC Root 2020 - Telekom Security TLS RSA Root 2023 Certificates removed: - Security Communication Root CA ##### Updated dependencies - ada updated to 2.7.6. - base64 updated to 0.5.2. - c-ares updated to 1.27.0. - corepack updated to 0.25.2. - ICU updated to 74.2. Includes CLDR 44.1 and Unicode 15.1. - npm updated to 10.5.0. Fixes a regression in signals not being passed onto child processes. - simdutf8 updated to 4.0.8. - Timezone updated to 2024a. - zlib updated to 1.3.0.1-motley-40e35a7. ##### vm: fix V8 compilation cache support for vm.Script Previously repeated compilation of the same source code using `vm.Script` stopped hitting the V8 compilation cache after v16.x when support for `importModuleDynamically` was added to `vm.Script`, resulting in a performance regression that blocked users (in particular Jest users) from upgrading from v16.x. The recent fixes allow the compilation cache to be hit again for `vm.Script` when `--experimental-vm-modules` is not used even in the presence of the `importModuleDynamically` option, so that users affected by the performance regression can now upgrade. Ongoing work is also being done to enable compilation cache support for `vm.CompileFunction`. Contributed by Joyee Cheung in [#49950](nodejs/node#49950) and [#50137](nodejs/node#50137). ##### Commits - \[[`c70383b8d4`](nodejs/node@c70383b8d4)] - **build**: support Python 3.12 (Shi Pujin) [#50209](nodejs/node#50209) - \[[`4b960c3a4a`](nodejs/node@4b960c3a4a)] - **build**: fix incorrect g++ warning message (Richard Lau) [#51695](nodejs/node#51695) - \[[`8fdea67694`](nodejs/node@8fdea67694)] - **crypto**: update root certificates to NSS 3.98 (Node.js GitHub Bot) [#51794](nodejs/node#51794) - \[[`812b126dd9`](nodejs/node@812b126dd9)] - **deps**: V8: cherry-pick [`d90d453`](nodejs/node@d90d4533b053) (Michaël Zasso) [#50077](nodejs/node#50077) - \[[`9ab8c3db87`](nodejs/node@9ab8c3db87)] - **deps**: update c-ares to 1.27.0 (Node.js GitHub Bot) [#51846](nodejs/node#51846) - \[[`c688680387`](nodejs/node@c688680387)] - **deps**: update c-ares to 1.26.0 (Node.js GitHub Bot) [#51582](nodejs/node#51582) - \[[`9498ac8a47`](nodejs/node@9498ac8a47)] - **deps**: compile c-ares with C11 support (Michaël Zasso) [#51410](nodejs/node#51410) - \[[`8fb743642f`](nodejs/node@8fb743642f)] - **deps**: update c-ares to 1.25.0 (Node.js GitHub Bot) [#51385](nodejs/node#51385) - \[[`7bea2d7c12`](nodejs/node@7bea2d7c12)] - **deps**: update zlib to 1.3.0.1-motley-40e35a7 (Node.js GitHub Bot) [#51274](nodejs/node#51274) - \[[`57a38c8f75`](nodejs/node@57a38c8f75)] - **deps**: update zlib to 1.3.0.1-motley-dd5fc13 (Node.js GitHub Bot) [#51105](nodejs/node#51105) - \[[`b0ca084a6b`](nodejs/node@b0ca084a6b)] - **deps**: update zlib to 1.3-22124f5 (Node.js GitHub Bot) [#50910](nodejs/node#50910) - \[[`4b43823f37`](nodejs/node@4b43823f37)] - **deps**: update zlib to 1.2.13.1-motley-5daffc7 (Node.js GitHub Bot) [#50803](nodejs/node#50803) - \[[`f0da591812`](nodejs/node@f0da591812)] - **deps**: update zlib to 1.2.13.1-motley-dfc48fc (Node.js GitHub Bot) [#50456](nodejs/node#50456) - \[[`16d28a883a`](nodejs/node@16d28a883a)] - **deps**: update base64 to 0.5.2 (Node.js GitHub Bot) [#51455](nodejs/node#51455) - \[[`13a9e81cb6`](nodejs/node@13a9e81cb6)] - **deps**: update base64 to 0.5.1 (Node.js GitHub Bot) [#50629](nodejs/node#50629) - \[[`b4502d3ac5`](nodejs/node@b4502d3ac5)] - **deps**: update simdutf to 4.0.8 (Node.js GitHub Bot) [#51000](nodejs/node#51000) - \[[`183cf8a74a`](nodejs/node@183cf8a74a)] - **deps**: update simdutf to 4.0.4 (Node.js GitHub Bot) [#50772](nodejs/node#50772) - \[[`11ba8593ea`](nodejs/node@11ba8593ea)] - **deps**: update ada to 2.7.6 (Node.js GitHub Bot) [#51542](nodejs/node#51542) - \[[`73a946d55c`](nodejs/node@73a946d55c)] - **deps**: update ada to 2.7.5 (Node.js GitHub Bot) [#51542](nodejs/node#51542) - \[[`cc434c1a39`](nodejs/node@cc434c1a39)] - **deps**: update ada to 2.7.4 (Node.js GitHub Bot) [#50815](nodejs/node#50815) - \[[`3a3808a6ae`](nodejs/node@3a3808a6ae)] - **deps**: upgrade npm to 10.5.0 (npm team) [#51913](nodejs/node#51913) - \[[`c8876d765c`](nodejs/node@c8876d765c)] - **deps**: upgrade npm to 10.3.0 (npm team) [#51431](nodejs/node#51431) - \[[`5aec3af460`](nodejs/node@5aec3af460)] - **deps**: update corepack to 0.25.2 (Node.js GitHub Bot) [#51810](nodejs/node#51810) - \[[`a593985326`](nodejs/node@a593985326)] - **deps**: update corepack to 0.24.1 (Node.js GitHub Bot) [#51459](nodejs/node#51459) - \[[`d1a9237bf5`](nodejs/node@d1a9237bf5)] - **deps**: update corepack to 0.24.0 (Node.js GitHub Bot) [#51318](nodejs/node#51318) - \[[`adac0c7a63`](nodejs/node@adac0c7a63)] - **deps**: update corepack to 0.23.0 (Node.js GitHub Bot) [#50563](nodejs/node#50563) - \[[`4a6f83e32a`](nodejs/node@4a6f83e32a)] - **deps**: escape Python strings correctly (Michaël Zasso) [#50695](nodejs/node#50695) - \[[`c13969e52a`](nodejs/node@c13969e52a)] - **deps**: V8: cherry-pick [`ea996ad`](nodejs/node@ea996ad04a68) (Nicolò Ribaudo) [#51136](nodejs/node#51136) - \[[`6fbf0ba5c3`](nodejs/node@6fbf0ba5c3)] - **deps**: V8: cherry-pick [`a0fd320`](nodejs/node@a0fd3209dda8) (Nicolò Ribaudo) [#51136](nodejs/node#51136) - \[[`68fd7516e1`](nodejs/node@68fd7516e1)] - **deps**: update timezone to 2024a (Michaël Zasso) [#51723](nodejs/node#51723) - \[[`f9b229ebe1`](nodejs/node@f9b229ebe1)] - **deps**: update icu to 74.2 (Michaël Zasso) [#51723](nodejs/node#51723) - \[[`90c73d2eb4`](nodejs/node@90c73d2eb4)] - **deps**: update timezone to 2023d (Node.js GitHub Bot) [#51461](nodejs/node#51461) - \[[`2a2bf57028`](nodejs/node@2a2bf57028)] - **deps**: update icu to 74.1 (Node.js GitHub Bot) [#50515](nodejs/node#50515) - \[[`425e011e52`](nodejs/node@425e011e52)] - **deps**: add v8::Object::SetInternalFieldForNodeCore() (Joyee Cheung) [#49874](nodejs/node#49874) - \[[`58c70344a2`](nodejs/node@58c70344a2)] - **deps**: V8: cherry-pick [`705e374`](nodejs/node@705e374124ae) (Joyee Cheung) [#51004](nodejs/node#51004) - \[[`b0e88899e1`](nodejs/node@b0e88899e1)] - **deps**: V8: cherry-pick [`1fada6b`](nodejs/node@1fada6b36f8d) (Joyee Cheung) [#51004](nodejs/node#51004) - \[[`d87a810b81`](nodejs/node@d87a810b81)] - **deps**: V8: cherry-pick [`3dd9576`](nodejs/node@3dd9576ce336) (Joyee Cheung) [#51004](nodejs/node#51004) - \[[`6d50966876`](nodejs/node@6d50966876)] - **deps**: V8: cherry-pick [`94e8282`](nodejs/node@94e8282325a1) (Joyee Cheung) [#51004](nodejs/node#51004) - \[[`fafbacdfec`](nodejs/node@fafbacdfec)] - **deps**: V8: cherry-pick [`9a98f96`](nodejs/node@9a98f96b6d68) (Joyee Cheung) [#51004](nodejs/node#51004) - \[[`d4a530ed8d`](nodejs/node@d4a530ed8d)] - **deps**: V8: cherry-pick [`7f5daed`](nodejs/node@7f5daed62d47) (Joyee Cheung) [#51004](nodejs/node#51004) - \[[`1ce901b164`](nodejs/node@1ce901b164)] - **deps**: V8: cherry-pick [`c400af4`](nodejs/node@c400af48b5ef) (Joyee Cheung) [#51004](nodejs/node#51004) - \[[`f232064f35`](nodejs/node@f232064f35)] - **doc**: fix historical experimental fetch flag (Kenrick) [#51506](nodejs/node#51506) - \[[`194ff6a40f`](nodejs/node@194ff6a40f)] - **(SEMVER-MINOR)** **doc**: add deprecation notice to `dirent.path` (Antoine du Hamel) [#50976](nodejs/node#50976) - \[[`0f09267dc6`](nodejs/node@0f09267dc6)] - **(SEMVER-MINOR)** **doc**: deprecate `dirent.path` (Antoine du Hamel) [#50976](nodejs/node#50976) - \[[`8bfb8f5b2f`](nodejs/node@8bfb8f5b2f)] - **doc,crypto**: further clarify RSA_PKCS1\_PADDING support (Tobias Nießen) [#51799](nodejs/node#51799) - \[[`c7baf7b274`](nodejs/node@c7baf7b274)] - **doc,crypto**: add changelog and note about disabled RSA_PKCS1\_PADDING (Filip Skokan) [#51782](nodejs/node#51782) - \[[`a193be3dc2`](nodejs/node@a193be3dc2)] - **esm**: use import attributes instead of import assertions (Antoine du Hamel) [#50140](nodejs/node#50140) - \[[`26e8f7793e`](nodejs/node@26e8f7793e)] - **(SEMVER-MINOR)** **fs**: introduce `dirent.parentPath` (Antoine du Hamel) [#50976](nodejs/node#50976) - \[[`5b5e5192f7`](nodejs/node@5b5e5192f7)] - **lib**: fix compileFunction throws range error for negative numbers (Jithil P Ponnan) [#49855](nodejs/node#49855) - \[[`7552de6806`](nodejs/node@7552de6806)] - **module**: fix the leak in SourceTextModule and ContextifySript (Joyee Cheung) [#48510](nodejs/node#48510) - \[[`2e05cf1c60`](nodejs/node@2e05cf1c60)] - **module**: fix leak of vm.SyntheticModule (Joyee Cheung) [#48510](nodejs/node#48510) - \[[`a86a2e14a3`](nodejs/node@a86a2e14a3)] - **module**: use symbol in WeakMap to manage host defined options (Joyee Cheung) [#48510](nodejs/node#48510) - \[[`32906ddcac`](nodejs/node@32906ddcac)] - **node-api**: segregate nogc APIs from rest via type system (Gabriel Schulhof) [#50060](nodejs/node#50060) - \[[`1aa71c26ff`](nodejs/node@1aa71c26ff)] - **node-api**: factor out common code into macros (Gabriel Schulhof) [#50664](nodejs/node#50664) - \[[`3d0b233f52`](nodejs/node@3d0b233f52)] - **node-api**: introduce experimental feature flags (Gabriel Schulhof) [#50991](nodejs/node#50991) - \[[`96514a8b9f`](nodejs/node@96514a8b9f)] - **src**: iterate on import attributes array correctly (Michaël Zasso) [#50703](nodejs/node#50703) - \[[`2c2892bf88`](nodejs/node@2c2892bf88)] - **src**: set ModuleWrap internal fields only once (Joyee Cheung) [#49391](nodejs/node#49391) - \[[`ff334cb774`](nodejs/node@ff334cb774)] - **src**: cast v8::Object::GetInternalField() return value to v8::Value (Joyee Cheung) [#48943](nodejs/node#48943) - \[[`270b519971`](nodejs/node@270b519971)] - **stream**: do not defer construction by one microtick (Matteo Collina) [#52005](nodejs/node#52005) - \[[`95d7a75084`](nodejs/node@95d7a75084)] - **test**: fix dns test case failures after c-ares update to 1.21.0+ (Brad House) [#50743](nodejs/node#50743) - \[[`cd613e5167`](nodejs/node@cd613e5167)] - **test**: handle relative https redirect (Richard Lau) [#51121](nodejs/node#51121) - \[[`40f10eafcf`](nodejs/node@40f10eafcf)] - **test**: fix `internet/test-inspector-help-page` (Richard Lau) [#51693](nodejs/node#51693) - \[[`5e426511b1`](nodejs/node@5e426511b1)] - **test**: deflake test-vm-contextified-script-leak (Joyee Cheung) [#49710](nodejs/node#49710) - \[[`0b156c6d28`](nodejs/node@0b156c6d28)] - **test**: use checkIfCollectable in vm leak tests (Joyee Cheung) [#49671](nodejs/node#49671) - \[[`1586c11b3c`](nodejs/node@1586c11b3c)] - **test**: add checkIfCollectable to test/common/gc.js (Joyee Cheung) [#49671](nodejs/node#49671) - \[[`902d8b3d4b`](nodejs/node@902d8b3d4b)] - **test**: fix flaky http-chunk-extensions-limit test (Ethan Arrowood) [#51943](nodejs/node#51943) - \[[`1743d2bdc1`](nodejs/node@1743d2bdc1)] - **test**: test surrogate pair filenames on windows (Mert Can Altın) [#51800](nodejs/node#51800) - \[[`1c1a7ec22d`](nodejs/node@1c1a7ec22d)] - **test**: increase platform timeout zlib-brotli-16gb (Rafael Gonzaga) [#51792](nodejs/node#51792) - \[[`931d02fe3e`](nodejs/node@931d02fe3e)] - **test, v8**: fix wrong import attributes test (Nicolò Ribaudo) [#52184](nodejs/node#52184) - \[[`d9ea6c1f8d`](nodejs/node@d9ea6c1f8d)] - **tls**: fix order of setting cipher before setting cert and key (Kumar Rishav) [#50186](nodejs/node#50186) - \[[`3184befa2e`](nodejs/node@3184befa2e)] - **tools**: fix update-icu.sh (Michaël Zasso) [#51723](nodejs/node#51723) - \[[`06646e11be`](nodejs/node@06646e11be)] - **(SEMVER-MINOR)** **vm**: use import attributes instead of import assertions (Antoine du Hamel) [#50141](nodejs/node#50141) - \[[`fe66e9d06e`](nodejs/node@fe66e9d06e)] - **vm**: reject in importModuleDynamically without --experimental-vm-modules (Joyee Cheung) [#50137](nodejs/node#50137) - \[[`052e095c6b`](nodejs/node@052e095c6b)] - **vm**: use internal versions of compileFunction and Script (Joyee Cheung) [#50137](nodejs/node#50137) - \[[`9f7899ed0a`](nodejs/node@9f7899ed0a)] - **vm**: unify host-defined option generation in vm.compileFunction (Joyee Cheung) [#50137](nodejs/node#50137) - \[[`6291c107d0`](nodejs/node@6291c107d0)] - **vm**: use default HDO when importModuleDynamically is not set (Joyee Cheung) [#49950](nodejs/node#49950) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Reviewed-on: https://harton.dev/cinder/cinder-space/pulls/25 Co-authored-by: Renovate Bot <bot@harton.nz> Co-committed-by: Renovate Bot <bot@harton.nz>

jgowdy-godaddy · 2024-05-10T23:24:51Z

Since the adoption of the policy whereby moving to a later version of Node-API
from an earlier version may entail rework of existing code, it is possible to
introduce modifications to already-released Node-APIs, as long as the
modifications affect neither the ABI nor the API of earlier versions. Such
modifications must be accompanied by an opt-out flag. This provides add-on
maintainers who take advantage of the initial compile-time flag to track
impending changes to Node-API with

a quick fix to the breakage caused,

a notification that such breakage is impending, and thus

a buffer to adoption above and beyond the one provided by the initial
compile-time flag.

Needing to introduce opt-out flags to fix breakage for updates in minor releases of Node LTS seems sub-optimal. Did the modification in this PR "affect neither the ABI nor the API of earlier versions"? It's difficult to maintain an ecosystem of Node-API addons if there will be breaking changes in minor releases of LTS.

gabrielschulhof requested a review from vmoroz October 6, 2023 06:43

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Oct 6, 2023

gabrielschulhof requested review from legendecas and mhdawson October 6, 2023 06:44

gabrielschulhof added the node-api Issues and PRs related to the Node-API. label Oct 6, 2023

gabrielschulhof force-pushed the type-segregate-pure-and-js-capable-apis branch 2 times, most recently from 42c4fad to 5c2ffc4 Compare October 6, 2023 07:16

gabrielschulhof mentioned this pull request Oct 6, 2023

idea: detect JS calls at compile time nodejs/node-addon-api#1367

Open

gabrielschulhof changed the title ~~node-api: segregate pure and non-pure APIs via type system~~ node-api: segregate nogc APIs from rest via type system Oct 6, 2023

gabrielschulhof force-pushed the type-segregate-pure-and-js-capable-apis branch from 5c2ffc4 to adc4664 Compare October 6, 2023 18:59

vmoroz reviewed Oct 6, 2023

View reviewed changes

test/js-native-api/test_finalizer/test_finalizer.c Outdated Show resolved Hide resolved

legendecas reviewed Oct 18, 2023

View reviewed changes

src/js_native_api.h Show resolved Hide resolved

src/node_api.h Show resolved Hide resolved

gabrielschulhof force-pushed the type-segregate-pure-and-js-capable-apis branch from ba34fd5 to a4bdd92 Compare October 27, 2023 16:21

vmoroz reviewed Nov 10, 2023

View reviewed changes

src/js_native_api_types.h Outdated Show resolved Hide resolved

vmoroz reviewed Nov 10, 2023

View reviewed changes

gabrielschulhof force-pushed the type-segregate-pure-and-js-capable-apis branch from a4bdd92 to fabdb5a Compare November 13, 2023 07:09

gabrielschulhof force-pushed the type-segregate-pure-and-js-capable-apis branch 4 times, most recently from ec70f12 to 708a319 Compare December 9, 2023 09:49

mhdawson approved these changes Dec 18, 2023

View reviewed changes

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 19, 2023

nodejs-github-bot merged commit 7a216d5 into nodejs:main Dec 19, 2023
54 checks passed

RafaelGSS mentioned this pull request Jan 2, 2024

v21.6.0 proposal #51342

Merged

This was referenced Feb 19, 2024

Release plan - v18.x Maintenance LTS nodejs/Release#737

Open

Release plan - v20.x Active LTS nodejs/Release#855

Open

gabrielschulhof added lts-watch-v18.x PRs that may need to be released in v18.x. lts-watch-v20.x PRs that may need to be released in v20.x labels Feb 19, 2024

gabrielschulhof mentioned this pull request Feb 19, 2024

[v18.x-backport] Backport feature flags and nogc types 2 #51804

Closed

gabrielschulhof removed the lts-watch-v18.x PRs that may need to be released in v18.x. label Feb 19, 2024

toyobayashi mentioned this pull request Feb 23, 2024

feat: segregate nogc APIs from rest via type system toyobayashi/emnapi#110

Merged

richardlau mentioned this pull request Mar 22, 2024

v18.20.0 proposal #52165

Merged

richardlau mentioned this pull request Mar 25, 2024

v20.12.0 proposal #52212

Merged

addaleax mentioned this pull request Mar 27, 2024

node-addon-api addons broken by default on Node.js 20.12+ #52229

Open

Alxe mentioned this pull request Apr 9, 2024

CON-311: Add warning to documentation rticommunity/rticonnextdds-connector-js#199

Merged

mhdawson mentioned this pull request May 7, 2024

Object wrap has a memory leak bug? nodejs/node-addon-api#1213

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

node-api: segregate nogc APIs from rest via type system #50060

node-api: segregate nogc APIs from rest via type system #50060

gabrielschulhof commented Oct 6, 2023 •

edited

nodejs-github-bot commented Oct 6, 2023 •

edited by gabrielschulhof

bnoordhuis commented Oct 6, 2023

gabrielschulhof commented Oct 6, 2023

bnoordhuis commented Oct 6, 2023

gabrielschulhof commented Oct 6, 2023 •

edited

gabrielschulhof commented Oct 6, 2023 •

edited

vmoroz Nov 10, 2023

gabrielschulhof Nov 10, 2023

vmoroz Nov 10, 2023

gabrielschulhof Nov 11, 2023

gabrielschulhof commented Nov 13, 2023 •

edited

mhdawson left a comment

nodejs-github-bot commented Dec 18, 2023

nodejs-github-bot commented Dec 19, 2023

jgowdy-godaddy commented May 10, 2024

node-api: segregate nogc APIs from rest via type system #50060

node-api: segregate nogc APIs from rest via type system #50060

Conversation

gabrielschulhof commented Oct 6, 2023 • edited

nodejs-github-bot commented Oct 6, 2023 • edited by gabrielschulhof

bnoordhuis commented Oct 6, 2023

gabrielschulhof commented Oct 6, 2023

bnoordhuis commented Oct 6, 2023

gabrielschulhof commented Oct 6, 2023 • edited

gabrielschulhof commented Oct 6, 2023 • edited

vmoroz Nov 10, 2023

Choose a reason for hiding this comment

gabrielschulhof Nov 10, 2023

Choose a reason for hiding this comment

vmoroz Nov 10, 2023

Choose a reason for hiding this comment

gabrielschulhof Nov 11, 2023

Choose a reason for hiding this comment

gabrielschulhof commented Nov 13, 2023 • edited

mhdawson left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Dec 18, 2023

nodejs-github-bot commented Dec 19, 2023

jgowdy-godaddy commented May 10, 2024

gabrielschulhof commented Oct 6, 2023 •

edited

nodejs-github-bot commented Oct 6, 2023 •

edited by gabrielschulhof

gabrielschulhof commented Oct 6, 2023 •

edited

gabrielschulhof commented Oct 6, 2023 •

edited

gabrielschulhof commented Nov 13, 2023 •

edited