Skip to content

src: add cleanup hooks to node::ObjectWrap#63642

Open
addaleax wants to merge 4 commits into
nodejs:mainfrom
addaleax:fix-addon-unload-objectwrap
Open

src: add cleanup hooks to node::ObjectWrap#63642
addaleax wants to merge 4 commits into
nodejs:mainfrom
addaleax:fix-addon-unload-objectwrap

Conversation

@addaleax
Copy link
Copy Markdown
Member

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-by tag.

src: add cleanup hooks to node::ObjectWrap

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: #63575
Fixes: #63540

test: add regression test for using ObjectWrap in worker

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.

addaleax and others added 2 commits May 29, 2026 13:39
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>
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/gyp

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels May 29, 2026
Comment thread test/addons/worker-addon-exit/test.js Outdated
if (!isMainThread) {
const addon = require(`./build/${common.buildType}/binding`);

// Create some garbage
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.

Is this necessary to test with the CleanupHook? I don't think it is necessary to trigger GC here, right?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

You're right, GC would prevent the test case from working, if anything – done in fcb0a4d

Copy link
Copy Markdown
Contributor

@mohd-akram mohd-akram May 29, 2026

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

@codecov
Copy link
Copy Markdown

codecov Bot commented May 29, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.33%. Comparing base (40dc5a1) to head (fcb0a4d).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #63642      +/-   ##
==========================================
+ Coverage   90.30%   90.33%   +0.03%     
==========================================
  Files         730      732       +2     
  Lines      234802   236357    +1555     
  Branches    43957    44491     +534     
==========================================
+ Hits       212041   213523    +1482     
- Misses      14485    14548      +63     
- Partials     8276     8286      +10     

see 43 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@addaleax addaleax added commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. addons Issues and PRs related to native addons. worker Issues and PRs related to Worker support. labels May 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

addons Issues and PRs related to native addons. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. needs-ci PRs that need a full CI run. worker Issues and PRs related to Worker support.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Segmentation fault on worker exit

4 participants