Skip to content

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

Closed
gabrielschulhof wants to merge 3 commits into
nodejs:v18.x-stagingfrom
gabrielschulhof:backport-feature-flags-and-nogc-types-2
Closed

[v18.x-backport] Backport feature flags and nogc types 2#51804
gabrielschulhof wants to merge 3 commits into
nodejs:v18.x-stagingfrom
gabrielschulhof:backport-feature-flags-and-nogc-types-2

Conversation

@gabrielschulhof

Copy link
Copy Markdown
Contributor

This backports the following PRs:

#50060
#50991
#50664

@nodejs-github-bot

nodejs-github-bot commented Feb 19, 2024

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/node-api
  • @nodejs/tsc

@nodejs-github-bot 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. v18.x labels Feb 19, 2024
gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Feb 19, 2024
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)
gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Feb 19, 2024
    * 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)
gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Feb 19, 2024
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 gabrielschulhof force-pushed the backport-feature-flags-and-nogc-types-2 branch from 263deaa to 65a8966 Compare February 19, 2024 07:39
@gabrielschulhof gabrielschulhof added the node-api Issues and PRs related to the Node-API. label Feb 19, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Feb 19, 2024
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 gabrielschulhof force-pushed the backport-feature-flags-and-nogc-types-2 branch from 65a8966 to 5bf1523 Compare February 19, 2024 18:17
@@ -84,7 +84,11 @@ define guards on the declaration of the new Node-API. Check for these guards
with:

```console

@gabrielschulhof gabrielschulhof Feb 19, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@mhdawson mhdawson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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)
@gabrielschulhof gabrielschulhof force-pushed the backport-feature-flags-and-nogc-types-2 branch from 5bf1523 to 344099d Compare March 1, 2024 05:50
@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 1, 2024
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 1, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

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

Copy link
Copy Markdown
Contributor Author

@mhdawson can you please give the PR another look?

@mhdawson mhdawson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Still LGTM

@gabrielschulhof

gabrielschulhof commented Mar 9, 2024

Copy link
Copy Markdown
Contributor Author

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

aduh95 commented Mar 9, 2024

Copy link
Copy Markdown
Contributor

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

@KevinEady

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor Author

@KevinEady yes, please also include #51801!

@richardlau

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member

@gabrielschulhof can you answer @richardlau 's question?

@richardlau

Copy link
Copy Markdown
Member

#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

Copy link
Copy Markdown
Contributor Author

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

richardlau pushed a commit that referenced this pull request Mar 22, 2024
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)
richardlau pushed a commit that referenced this pull request Mar 22, 2024
    * 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)
richardlau pushed a commit that referenced this pull request Mar 22, 2024
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

richardlau commented Mar 22, 2024

Copy link
Copy Markdown
Member

Landed in 931d02f...32906dd.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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. node-api Issues and PRs related to the Node-API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants