[v24.x backport] src: keep global list of addon-provided cleanup hooks - #65042
Open
imechZhangLY wants to merge 1 commit into
Open
[v24.x backport] src: keep global list of addon-provided cleanup hooks#65042imechZhangLY wants to merge 1 commit into
imechZhangLY wants to merge 1 commit into
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> PR-URL: nodejs#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> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
nodejs-github-bot
added
c++
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
3 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.
Backports #63985 to v24.x. The fix is needed after 4b5eb7b was included in v24.19.0 without the corresponding follow-up, which can make ObjectWrap destruction abort in RemoveEnvironmentCleanupHook() when no Node.js Environment is current.\n\nThe test conflict was resolved by retaining v24.x's existing context->GetIsolate() usage.\n\nRefs: #63985\nFixes: #63923\n\nValidation: tools/cpplint.py passes for both changed C++ files. A local Windows build could not be run because the installed Visual Studio environment does not include the required Clang/LLVM toolset.