Skip to content

src: fix libuv assertion on windows#61999

Open
liuxingbaoyu wants to merge 2 commits intonodejs:mainfrom
liuxingbaoyu:fix-56645
Open

src: fix libuv assertion on windows#61999
liuxingbaoyu wants to merge 2 commits intonodejs:mainfrom
liuxingbaoyu:fix-56645

Conversation

@liuxingbaoyu
Copy link
Contributor

Set the pointer to nullptr after calling uv_close to avoid assertions.

Fixes: #56645

@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 Feb 26, 2026
Set the pointer to `nullptr` after calling `uv_close`
to avoid assertions.

Fixes: nodejs#56645
@juanarbol
Copy link
Member

That handle is gonna leak, and I don't think it's gonna fix anything.

This looks pretty much a AI-generated solution... that does actually nothing regarding your intention. 💔

@liuxingbaoyu
Copy link
Contributor Author

This is not an AI-generated solution, it's an inspiration I got from other code.

node/src/node_platform.cc

Lines 411 to 436 in a8eb690

void PerIsolatePlatformData::Shutdown() {
auto foreground_tasks_locked = foreground_tasks_.Lock();
auto foreground_delayed_tasks_locked = foreground_delayed_tasks_.Lock();
foreground_delayed_tasks_locked.PopAll();
foreground_tasks_locked.PopAll();
scheduled_delayed_tasks_.clear();
if (flush_tasks_ != nullptr) {
// Both destroying the scheduled_delayed_tasks_ lists and closing
// flush_tasks_ handle add tasks to the event loop. We keep a count of all
// non-closed handles, and when that reaches zero, we inform any shutdown
// callbacks that the platform is done as far as this Isolate is concerned.
self_reference_ = shared_from_this();
uv_close(reinterpret_cast<uv_handle_t*>(flush_tasks_),
[](uv_handle_t* handle) {
std::unique_ptr<uv_async_t> flush_tasks{
reinterpret_cast<uv_async_t*>(handle)};
PerIsolatePlatformData* platform_data =
static_cast<PerIsolatePlatformData*>(flush_tasks->data);
platform_data->DecreaseHandleCount();
platform_data->self_reference_.reset();
});
flush_tasks_ = nullptr;
}
}

@codecov
Copy link

codecov bot commented Feb 26, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.73%. Comparing base (488a854) to head (a53efb0).
⚠️ Report is 34 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #61999      +/-   ##
==========================================
- Coverage   89.77%   89.73%   -0.05%     
==========================================
  Files         674      676       +2     
  Lines      205705   206068     +363     
  Branches    39449    39514      +65     
==========================================
+ Hits       184670   184907     +237     
- Misses      13280    13312      +32     
- Partials     7755     7849      +94     
Files with missing lines Coverage Δ
src/node_platform.cc 76.19% <100.00%> (-0.06%) ⬇️

... and 51 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.

@juanarbol
Copy link
Member

This is not an AI-generated solution, it's an inspiration I got from other code.

My bad... but sadly still... I don't think this is gonna fix the fundamental problem at all and still leaking.

double delay_in_seconds) {
auto locked = tasks_.Lock();

if (flush_tasks_ == nullptr) return;
Copy link
Member

@addaleax addaleax Feb 26, 2026

Choose a reason for hiding this comment

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

If "just not sending" is indeed a valid solution to this issue (which I have not verified), you could set a boolean flag next to flush_tasks_ instead of making it a heap-allocated variable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

v8 is calling PostDelayedTaskOnWorkerThread here, which looks like a task for wasm caching.
https://github.com/v8/v8/blob/1fb9a7b9671a724ebdcf57db3807d4af56dc6cbb/src/wasm/module-compiler.cc#L4004-L4011
Ignoring it should be safe, because anyway, the delay here is 2 seconds, and the node will have finished long before then.

@liuxingbaoyu
Copy link
Contributor Author

@juanarbol I understand now. The AI ​​is telling me that the handle wasn't deleted. AI ​​is more familiar with C++ than I am. 🤦‍♂️

@addaleax addaleax added the review wanted PRs that need reviews. label Mar 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. review wanted PRs that need reviews.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

libuv assertion on Windows with Node.js 23.x

4 participants