src: keep global list of addon-provided cleanup hooks#63985
Closed
addaleax wants to merge 1 commit into
Closed
Conversation
A recent change, 215027c, introduced flakiness into our test suite that exposed an issue with the cleanup hook API design. Specifically, the signatures of `AddEnvironmentCleanupHook()` and `RemoveEnvironmentCleanupHook()` are problematic. Both functions take `Isolate*` arguments, as addons are not generally expected to have to care about the Node.js `Environment` as a first-class scope provider. However, this model made the incorrect assumption that in the situations in which `RemoveEnvironmentCleanupHook()` would be invoked an `Environment` would always be associated with the current `Isolate` (via the current V8 `Context`, if there is one). This occasionally breaks down when `RemoveEnvironmentCleanupHook()` is called during garbage collection -- which would be an expected use case of the functionality, but one that has not been covered through our tests before 215027c. Since Node.js guarantees API and ABI stability within a major version, and this is a bug that is independent from the aforementioned change, this commit resolves it by adding global mutable state to keep track off cleanup hooks registered through the Node.js public API. Obviously, this solution does not represent a desirable long-term state, and a semver-minor follow up should add an API that does not require modifications to these data structures, likely based on the async cleanup hook API which already solves this issue properly. Refs: nodejs#63642 Fixes: nodejs#63923 Signed-off-by: Anna Henningsen <anna@addaleax.net>
nodejs-github-bot
added
c++
jasnell
approved these changes
Jun 19, 2026
addaleax
added
request-ci
github-actions
Bot
removed
the
request-ci
This comment was marked as outdated.
This comment was marked as outdated.
Collaborator
santigimeno
approved these changes
Jun 19, 2026
mcollina
added
the
commit-queue
Member
|
The windows coverage job is now skipped on main |
nodejs-github-bot
added
commit-queue-failed
Collaborator
Commit Queue failed- Loading data for nodejs/node/pull/63985 ✔ Done loading data for nodejs/node/pull/63985 ----------------------------------- PR info ------------------------------------ Title src: keep global list of addon-provided cleanup hooks (#63985) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch addaleax:fix-63923 -> nodejs:main Labels c++, author ready, needs-ci Commits 1 - src: keep global list of addon-provided cleanup hooks Committers 1 - Anna Henningsen <anna@addaleax.net> PR-URL: https://github.com/nodejs/node/pull/63985 Fixes: https://github.com/nodejs/node/issues/63923 Refs: https://github.com/nodejs/node/pull/63642 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/63985 Fixes: https://github.com/nodejs/node/issues/63923 Refs: https://github.com/nodejs/node/pull/63642 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> -------------------------------------------------------------------------------- ℹ This PR was created on Thu, 18 Jun 2026 16:04:55 GMT ✔ Approvals: 3 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/63985#pullrequestreview-4530362095 ✔ - Santiago Gimeno (@santigimeno): https://github.com/nodejs/node/pull/63985#pullrequestreview-4532738398 ✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/63985#pullrequestreview-4537144735 ✘ 1 GitHub CI job(s) failed: ✘ - coverage-windows: FAILURE (https://github.com/nodejs/node/actions/runs/27772695943/job/82176628881) ℹ Last Full PR CI on 2026-06-19T12:10:44Z: https://ci.nodejs.org/job/node-test-pull-request/74255/ - Querying data for job/node-test-pull-request/74255/ ✔ Build data downloaded ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/27876670330 |
addaleax
added a commit
that referenced
this pull request
Jun 20, 2026
A recent change, 215027c, introduced flakiness into our test suite that exposed an issue with the cleanup hook API design. Specifically, the signatures of `AddEnvironmentCleanupHook()` and `RemoveEnvironmentCleanupHook()` are problematic. Both functions take `Isolate*` arguments, as addons are not generally expected to have to care about the Node.js `Environment` as a first-class scope provider. However, this model made the incorrect assumption that in the situations in which `RemoveEnvironmentCleanupHook()` would be invoked an `Environment` would always be associated with the current `Isolate` (via the current V8 `Context`, if there is one). This occasionally breaks down when `RemoveEnvironmentCleanupHook()` is called during garbage collection -- which would be an expected use case of the functionality, but one that has not been covered through our tests before 215027c. Since Node.js guarantees API and ABI stability within a major version, and this is a bug that is independent from the aforementioned change, this commit resolves it by adding global mutable state to keep track off cleanup hooks registered through the Node.js public API. Obviously, this solution does not represent a desirable long-term state, and a semver-minor follow up should add an API that does not require modifications to these data structures, likely based on the async cleanup hook API which already solves this issue properly. Refs: #63642 Fixes: #63923 Signed-off-by: Anna Henningsen <anna@addaleax.net> PR-URL: #63985 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Member
Author
|
Landed in 68321ef |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A recent change, 215027c, introduced flakiness into our test suite that exposed an issue with the cleanup hook API design.
Specifically, the signatures of
AddEnvironmentCleanupHook()andRemoveEnvironmentCleanupHook()are problematic. Both functions takeIsolate*arguments, as addons are not generally expected to have to care about the Node.jsEnvironmentas a first-class scope provider.However, this model made the incorrect assumption that in the situations in which
RemoveEnvironmentCleanupHook()would be invoked anEnvironmentwould always be associated with the currentIsolate(via the current V8Context, if there is one).This occasionally breaks down when
RemoveEnvironmentCleanupHook()is called during garbage collection -- which would be an expected use case of the functionality, but one that has not been covered through our tests before 215027c.Since Node.js guarantees API and ABI stability within a major version, and this is a bug that is independent from the aforementioned change, this commit resolves it by adding global mutable state to keep track off cleanup hooks registered through the Node.js public API.
Obviously, this solution does not represent a desirable long-term state, and a semver-minor follow up should add an API that does not require modifications to these data structures, likely based on the async cleanup hook API which already solves this issue properly.
Refs: #63642
Fixes: #63923