Skip to content

src: fix libuv assertion on windows#61999

Open
liuxingbaoyu wants to merge 1 commit intonodejs:mainfrom
liuxingbaoyu:fix-56645
Open

src: fix libuv assertion on windows#61999
liuxingbaoyu wants to merge 1 commit 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

❌ Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 89.73%. Comparing base (488a854) to head (b721fef).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
src/node_platform.cc 88.88% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #61999      +/-   ##
==========================================
- Coverage   89.77%   89.73%   -0.05%     
==========================================
  Files         674      676       +2     
  Lines      205705   205987     +282     
  Branches    39449    39487      +38     
==========================================
+ Hits       184670   184833     +163     
- Misses      13280    13300      +20     
- Partials     7755     7854      +99     
Files with missing lines Coverage Δ
src/node_platform.cc 76.23% <88.88%> (-0.01%) ⬇️

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

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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

libuv assertion on Windows with Node.js 23.x

4 participants