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

gabrielschulhof · 2024-02-19T07:37:58Z

This backports the following PRs:

nodejs-github-bot · 2024-02-19T07:38:03Z

Review requested:

@nodejs/node-api
@nodejs/tsc

Add a flag for each experimental feature to indicate its presence. That way, if we compile with `NAPI_EXPERIMENTAL` turned on, we'll be able to distinguish between what `NAPI_EXPERIMENTAL` used to mean on an old version of the headers when compiling against such an old version, and what it means on a new version of Node.js. PR-URL: nodejs#50991 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Vladimir Morozov <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Backport-PR-URL: nodejs#51804 (cherry picked from commit 727dd28)

* Create macro for checking new string arguments. * Create macro for combining env check and inside-gc check. PR-URL: nodejs#50664 Reviewed-By: Vladimir Morozov <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Backport-PR-URL: nodejs#51804 (cherry picked from commit 5e250bd)

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 <[email protected]> Reviewed-By: Vladimir Morozov <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Backport-PR-URL: nodejs#51804 (cherry picked from commit 7a216d5)

nodejs-github-bot · 2024-02-19T07:42:41Z

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

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 <[email protected]> Reviewed-By: Vladimir Morozov <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Backport-PR-URL: nodejs#51804 (cherry picked from commit 7a216d5)

gabrielschulhof · 2024-02-19T18:18:48Z

doc/contributing/releases-node-api.md

@@ -84,7 +84,11 @@ define guards on the declaration of the new Node-API. Check for these guards
 with:

 ```console


This was the only conflict. On main we had moved to ```bash, so the context of the hunk was different.

nodejs-github-bot · 2024-02-21T09:50:40Z

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

mhdawson

LGTM

Add a flag for each experimental feature to indicate its presence. That way, if we compile with `NAPI_EXPERIMENTAL` turned on, we'll be able to distinguish between what `NAPI_EXPERIMENTAL` used to mean on an old version of the headers when compiling against such an old version, and what it means on a new version of Node.js. PR-URL: nodejs#50991 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Vladimir Morozov <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Backport-PR-URL: nodejs#51804 (cherry picked from commit 727dd28)

* Create macro for checking new string arguments. * Create macro for combining env check and inside-gc check. PR-URL: nodejs#50664 Reviewed-By: Vladimir Morozov <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Backport-PR-URL: nodejs#51804 (cherry picked from commit 5e250bd)

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 <[email protected]> Reviewed-By: Vladimir Morozov <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Backport-PR-URL: nodejs#51804 (cherry picked from commit 7a216d5)

nodejs-github-bot · 2024-03-01T12:57:08Z

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

nodejs-github-bot · 2024-03-07T15:55:51Z

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

gabrielschulhof · 2024-03-08T16:46:39Z

@mhdawson can you please give the PR another look?

mhdawson

Still LGTM

gabrielschulhof · 2024-03-09T09:20:27Z

Hmmm ... it looks like the commit queue doesn't work with this branch, and

remote: error: GH006: Protected branch update failed for refs/heads/v18.x-staging.
remote: error: You're not authorized to push to this branch. Visit https://docs.github.com/articles/about-protected-branches/ for more information.

if I try to use git node land.

@mhdawson, can you land this PR?

aduh95 · 2024-03-09T20:25:04Z

node/doc/contributing/collaborator-guide.md

Lines 801 to 802 in b1468d2

    
           Only members of @nodejs/backporters should land commits onto LTS staging 
        
           branches.

KevinEady · 2024-03-11T09:39:38Z

Hi @gabrielschulhof ,

Should #51801 also be included in this backport? It looks like napi_create_threadsafe_function still uses node_api_nogc_finalize nogc_thread_finalize_cb instead of napi_finalize.

gabrielschulhof · 2024-03-11T15:49:20Z

@KevinEady yes, please also include #51801!

richardlau · 2024-03-15T17:34:51Z

Should #51801 also be included in this backport? It looks like napi_create_threadsafe_function still uses node_api_nogc_finalize nogc_thread_finalize_cb instead of napi_finalize.

@KevinEady yes, please also include #51801!

Is this waiting on #51801 to be including in this backport PR?

mhdawson · 2024-03-18T16:53:33Z

@gabrielschulhof can you answer @richardlau 's question?

richardlau · 2024-03-20T13:41:18Z

#51801 hasn't gone out in a current release yet, so is technically not eligible to land on LTS.

Does it make more sense to delay this PR so that it can land together with #51801, or should we land this PR first on v18.x without #51801?

gabrielschulhof · 2024-03-22T15:12:26Z

@richardlau this PR can land on v18.x on its own. The subsequent PR can land after that. It is an improvement over this PR, but PR does provide value on its own.

Add a flag for each experimental feature to indicate its presence. That way, if we compile with `NAPI_EXPERIMENTAL` turned on, we'll be able to distinguish between what `NAPI_EXPERIMENTAL` used to mean on an old version of the headers when compiling against such an old version, and what it means on a new version of Node.js. PR-URL: #50991 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Vladimir Morozov <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Backport-PR-URL: #51804 (cherry picked from commit 727dd28)

* Create macro for checking new string arguments. * Create macro for combining env check and inside-gc check. PR-URL: #50664 Reviewed-By: Vladimir Morozov <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Backport-PR-URL: #51804 (cherry picked from commit 5e250bd)

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 <[email protected]> Reviewed-By: Vladimir Morozov <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Backport-PR-URL: #51804 (cherry picked from commit 7a216d5)

richardlau · 2024-03-22T16:32:10Z

Landed in 931d02f...32906dd.

gabrielschulhof force-pushed the backport-feature-flags-and-nogc-types-2 branch from 263deaa to 65a8966 Compare February 19, 2024 07:39

gabrielschulhof added the node-api Issues and PRs related to the Node-API. label Feb 19, 2024

gabrielschulhof requested review from vmoroz, legendecas and mhdawson February 19, 2024 07:39

gabrielschulhof mentioned this pull request Feb 19, 2024

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

Open

gabrielschulhof force-pushed the backport-feature-flags-and-nogc-types-2 branch from 65a8966 to 5bf1523 Compare February 19, 2024 18:17

gabrielschulhof commented Feb 19, 2024

View reviewed changes

mhdawson approved these changes Feb 29, 2024

View reviewed changes

gabrielschulhof added 3 commits February 29, 2024 21:49

gabrielschulhof force-pushed the backport-feature-flags-and-nogc-types-2 branch from 5bf1523 to 344099d Compare March 1, 2024 05:50

richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 1, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 1, 2024

gabrielschulhof added commit-queue Add this label to land a pull request using GitHub Actions. and removed needs-ci PRs that need a full CI run. labels Mar 8, 2024

mhdawson approved these changes Mar 8, 2024

View reviewed changes

aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Mar 9, 2024

richardlau closed this Mar 22, 2024

richardlau mentioned this pull request Apr 2, 2024

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

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

gabrielschulhof commented Feb 19, 2024

nodejs-github-bot commented Feb 19, 2024 •

edited by gabrielschulhof

Loading

nodejs-github-bot commented Feb 19, 2024

gabrielschulhof Feb 19, 2024 •

edited

Loading

nodejs-github-bot commented Feb 21, 2024

mhdawson left a comment

nodejs-github-bot commented Mar 1, 2024

nodejs-github-bot commented Mar 7, 2024

gabrielschulhof commented Mar 8, 2024

mhdawson left a comment

gabrielschulhof commented Mar 9, 2024 •

edited

Loading

aduh95 commented Mar 9, 2024

KevinEady commented Mar 11, 2024

gabrielschulhof commented Mar 11, 2024

richardlau commented Mar 15, 2024

mhdawson commented Mar 18, 2024

richardlau commented Mar 20, 2024

gabrielschulhof commented Mar 22, 2024

richardlau commented Mar 22, 2024 •

edited

Loading

		@@ -84,7 +84,11 @@ define guards on the declaration of the new Node-API. Check for these guards
		with:

		```console

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

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

Conversation

gabrielschulhof commented Feb 19, 2024

nodejs-github-bot commented Feb 19, 2024 • edited by gabrielschulhof Loading

nodejs-github-bot commented Feb 19, 2024

gabrielschulhof Feb 19, 2024 • edited Loading

Choose a reason for hiding this comment

nodejs-github-bot commented Feb 21, 2024

mhdawson left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Mar 1, 2024

nodejs-github-bot commented Mar 7, 2024

gabrielschulhof commented Mar 8, 2024

mhdawson left a comment

Choose a reason for hiding this comment

gabrielschulhof commented Mar 9, 2024 • edited Loading

aduh95 commented Mar 9, 2024

KevinEady commented Mar 11, 2024

gabrielschulhof commented Mar 11, 2024

richardlau commented Mar 15, 2024

mhdawson commented Mar 18, 2024

richardlau commented Mar 20, 2024

gabrielschulhof commented Mar 22, 2024

richardlau commented Mar 22, 2024 • edited Loading

nodejs-github-bot commented Feb 19, 2024 •

edited by gabrielschulhof

Loading

gabrielschulhof Feb 19, 2024 •

edited

Loading

gabrielschulhof commented Mar 9, 2024 •

edited

Loading

richardlau commented Mar 22, 2024 •

edited

Loading