Skip to content

src: fix task queue deadlock when built as C++23 - #66066

Open
codebytere wants to merge 1 commit into
nodejs:mainfrom
codebytere:platform-popall-lock-scope
Open

codebytere wants to merge 1 commit into
nodejs:mainfrom
codebytere:platform-popall-lock-scope

Conversation

@codebytere

Copy link
Copy Markdown
Member

Refs: #65353

Since #65353, FlushForegroundTasksInternal() and the worker DelayedTaskScheduler drain their queues with for (auto& task : queue.Lock().PopAll()). In C++20 the Locked temporary is destroyed at the end of the range initializer, before the loop body runs. C++23 (P2718R0) extends the lifetime of every temporary in a range-for initializer to the end of the loop, so the queue's mutex stays held while the tasks run, and the first task that posts to the same queue - any V8 foreground task that schedules another one - deadlocks on the non-recursive mutex. Node.js itself builds with -std=gnu++20 and is unaffected today, but an embedder that compiles it as C++23 hangs in the first foreground task flush, and so will Node.js once common.gypi moves on.

This stores the drained tasks in a local before iterating at the three sites, so the lock is released independent of the language version, and notes the constraint on PopAll(). No new test: PlatformTest.SkipNewTasksInFlushForegroundTasks already posts a foreground task from inside a flushed one and deadlocks under a C++23 build without this change; verified with a clang 20 -std=gnu++23 build of main before (hangs in test-worker-arraybuffer-zerofill and that cctest) and after (passes), plus the regular gcc C++20 build.


Disclosure: the code and this description were written by Claude Code, directed and reviewed by @codebytere.

@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 Sep 16, 2026
Since nodejs#65353, FlushForegroundTasksInternal() and the
DelayedTaskScheduler drain their queues with
`for (auto& task : queue.Lock().PopAll())`. In C++20 the Locked
temporary is destroyed at the end of the range initializer, before the
loop body runs. C++23 (P2718R0) extends the lifetime of every temporary
in a range-for initializer to the end of the loop, so the queue's mutex
stays held while the tasks run, and the first task that posts to the
same queue - any V8 foreground task that schedules another one -
deadlocks on the non-recursive mutex. Node.js itself builds with
-std=gnu++20, but an embedder that compiles it as C++23 hangs in the
first foreground task flush.

Store the drained tasks in a local before iterating so the lock is
released independent of the language version.

Refs: nodejs#65353
Signed-off-by: Shelley Vohr <shelley.vohr@gmail.com>
@codebytere
codebytere force-pushed the platform-popall-lock-scope branch from bb29cbb to 8e9431e Compare September 16, 2026 13:22
@codecov

codecov Bot commented Sep 16, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.26%. Comparing base (67e66b8) to head (8e9431e).

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #66066      +/-   ##
==========================================
- Coverage   90.27%   90.26%   -0.01%     
==========================================
  Files         789      789              
  Lines      271473   271475       +2     
  Branches    51808    51806       -2     
==========================================
- Hits       245066   245045      -21     
- Misses      16880    16897      +17     
- Partials     9527     9533       +6     
Files with missing lines Coverage Δ
src/node_platform.cc 75.04% <100.00%> (+0.09%) ⬆️
src/node_platform.h 85.71% <ø> (ø)

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

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.

3 participants