src: add cleanup hooks to node::ObjectWrap#63642
Conversation
Add cleanup hooks to make sure that addons making use of this legacy API can do so safely in Worker threads or embedder-controlled Node.js instances. Refs: nodejs#63575 Fixes: nodejs#63540 Signed-off-by: Anna Henningsen <anna@addaleax.net>
This content is taken from nodejs#63575. The original commit message is preserved below: --- worker: fix premature addon unload Weak callbacks could run after addons were unloaded, leading to a crash. Signed-off-by: Mohamed Akram <mohd.akram@outlook.com>
|
Review requested:
|
nodejs-github-bot
added
c++
| if (!isMainThread) { | ||
| const addon = require(`./build/${common.buildType}/binding`); | ||
|
|
||
| // Create some garbage |
There was a problem hiding this comment.
Is this necessary to test with the CleanupHook? I don't think it is necessary to trigger GC here, right?
There was a problem hiding this comment.
You're right, GC would prevent the test case from working, if anything – done in fcb0a4d
There was a problem hiding this comment.
@legendecas @addaleax It's not meant to trigger a GC immediately. The purpose of the loop is to make sure the WeakCallback runs, which it does after the addon is unloaded, and therefore causes a segfault absent a fix. Without that loop you won't be testing the visible effects of the clean up hook (i.e. that it removes the WeakCallback). The loop iterations were chosen such that this happens consistently.
There was a problem hiding this comment.
@mohd-akram If the weak callback runs here, won't that mean that the object is destroyed before the Environment and therefore the issue in #63540 will actually not occur? I can reproduce a fairly consistent crash with current Node.js versions (i.e. before this PR) when the loop is commented out and no consistent crash when it is active.
There was a problem hiding this comment.
If the weak callback runs here, won't that mean that the object is destroyed before the Environment and therefore the issue in #63540 will actually not occur?
On my system, it doesn't run there, it runs after.
I can reproduce a fairly consistent crash with current Node.js versions (i.e. before this PR) when the loop is commented out and no consistent crash when it is active.
Hmm, that's surprising. Definitely the opposite happening for me:
$ cat test.js
'use strict';
const common = require('../../common');
const { isMainThread, Worker } = require('worker_threads');
if (!isMainThread) {
const addon = require(`./build/${common.buildType}/binding`);
// Create some garbage
const arr = [];
for (let i = 0; i < 1e5; i++) arr.push(`${i}`.repeat(100));
new addon.MyObject(10);
// Should not segfault
throw new Error('exit');
} else {
const worker = new Worker(__filename);
worker.on('error', () => {});
}
$ node --version
v26.2.0
$ node test.js
zsh: segmentation fault node test.js
What OS are you on?
There was a problem hiding this comment.
It's arm64 macOS.
Testing out a few different combinations: Node.js 26.0.0 has this crash only without the extra array, Node.js 26.1.0 and 26.2.0 do not have it at all, and Node.js @ 40dc5a1 (base of this PR) has it only with the extra array.
I could probably dig deeper into the exact reasons, but regardless of what the outcome of that would be, I don't really see a good reason not to just include both versions as tests, so I've done that in 0cede86
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #63642 +/- ##
==========================================
+ Coverage 90.30% 90.32% +0.01%
==========================================
Files 730 732 +2
Lines 234802 236435 +1633
Branches 43957 44531 +574
==========================================
+ Hits 212041 213560 +1519
- Misses 14485 14596 +111
- Partials 8276 8279 +3 🚀 New features to boost your workflow:
|
addaleax
added
commit-queue-rebase
github-actions
Bot
removed
the
request-ci
addaleax
added
the
commit-queue
nodejs-github-bot
removed
the
commit-queue
|
Landed in 3518f93...be7ea27 |
This content is taken from #63575. The original commit message is preserved below: --- worker: fix premature addon unload Weak callbacks could run after addons were unloaded, leading to a crash. Signed-off-by: Mohamed Akram <mohd.akram@outlook.com> PR-URL: #63642 Fixes: #63540 Refs: #63575 Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
|
https://github.com/nodejs/node/actions/runs/27140598797/job/80104749457
|
|
@addaleax Will this commit be backported to Node 24? #63540 (comment) mentioned that the crash also happens on Node 24. Thank you! |
This content is taken from #63575. The original commit message is preserved below: --- worker: fix premature addon unload Weak callbacks could run after addons were unloaded, leading to a crash. Signed-off-by: Mohamed Akram <mohd.akram@outlook.com> PR-URL: #63642 Fixes: #63540 Refs: #63575 Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
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>
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>
6 participants
In two commits to keep attribution to @mohd-akram's PR intact. If somebody feels strongly that they should be a single commit, I can turn them into one with a
Co-authored-bytag.src: add cleanup hooks to
node::ObjectWrapAdd cleanup hooks to make sure that addons making use
of this legacy API can do so safely in Worker threads
or embedder-controlled Node.js instances.
Refs: #63575
Fixes: #63540
test: add regression test for using
ObjectWrapin workerThis content is taken from #63575.
The original commit message is preserved below: