Skip to content

src: run same-priority platform tasks in posting order - #65353

Open
codebytere wants to merge 1 commit into
nodejs:mainfrom
codebytere:fix/platform-task-queue-fifo
Open

src: run same-priority platform tasks in posting order#65353
codebytere wants to merge 1 commit into
nodejs:mainfrom
codebytere:fix/platform-task-queue-fifo

Conversation

@codebytere

@codebytere codebytere commented Aug 17, 2026

Copy link
Copy Markdown
Member

TaskQueue<T> has been a std::priority_queue since #58047 so that worker tasks honor v8::TaskPriority. Its
comparator returns false for entry types without a priority member and for equal priorities, and the comment expects
that to keep insertion order. A binary heap doesn't: three tasks pushed A, B, C pop as A, C, B, and 64 pushes come back
out as 0, 2, 6, 14, 30, 62, ...

Three queues are affected: the per-isolate foreground queue (tasks of one priority no longer run in the order they were
posted), the foreground delayed queue, and the worker runner's DelayedTaskScheduler, where it can hang shutdown. That
scheduler drains its local queue in one batch, so when V8 posts a delayed worker task shortly before the platform shuts
down, the StopTask pushed by Stop() can run before a ScheduleTask that was pushed earlier; the ScheduleTask then
starts a timer on the scheduler's loop after all timers were stopped, and Shutdown() sits in uv_thread_join() until
the delay expires (8 s when the late task is the memory reducer). It shows up as an intermittent multi-second exit stall
in processes that exit soon after doing some work.

This is the other half of the shutdown race #61999 addressed: that change makes PostDelayedTask() return early once
Stop() has run (both under the tasks_ lock), so every ScheduleTask that is queued was queued before the StopTask;
with posting order restored it also runs before it, and no further flag is needed.

The fix gives every queued item a sequence number and uses it as the tie breaker, so equal-priority and priority-less
tasks come out FIFO again while higher priorities still go first; PopAll() returns the tasks in that order instead of
handing out the heap, which also removes the const_cast loops at its three call sites.

Tests: new cctest (TaskQueueTest.HigherPriorityFirstThenPostingOrder: FIFO across PopAll()/Pop() for a
priority-less queue, priority-then-FIFO for TaskQueueEntry) fails on main and passes here; cctest and the default
suite pass.

Refs: #58047
Refs: #61999


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

TaskQueue became a std::priority_queue when worker tasks started to
honor v8::TaskPriority. Its comparator returns false for entry types
without a priority member, and for entries of equal priority, on the
assumption that the heap then keeps insertion order. It does not: three
tasks pushed A, B, C pop as A, C, B, and larger batches come out in
heap order. That affects the per-isolate foreground task queue (tasks
of one priority no longer run in the order they were posted), the
foreground delayed task queue, and the delayed task scheduler of the
worker thread task runner, whose local queue is drained in one batch:
when v8 posts a delayed worker task shortly before the platform shuts
down, the StopTask pushed by Stop() can run before a ScheduleTask that
was pushed earlier, that ScheduleTask then starts a timer on the
scheduler's loop after all timers were supposed to be stopped, and
Shutdown() blocks in uv_thread_join() until the delay (e.g. the 8 s of
the memory reducer) expires.

Give every queued item a sequence number and use it as the tie
breaker, so that tasks of equal priority, and tasks without one, come
out in FIFO order again; higher priorities still come first. PopAll()
now returns the tasks in that order instead of handing out the heap.

Signed-off-by: Shelley Vohr <shelley.vohr@gmail.com>
@codebytere codebytere added request-ci Add this label to start a Jenkins CI on a PR. c++ Issues and PRs that require attention from people who are familiar with C++. labels Aug 17, 2026
@codecov

codecov Bot commented Aug 17, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.12%. Comparing base (30bff4a) to head (121482b).
⚠️ Report is 27 commits behind head on main.

Files with missing lines Patch % Lines
src/node_platform.cc 94.73% 0 Missing and 1 partial ⚠️
src/node_platform.h 80.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #65353      +/-   ##
==========================================
- Coverage   90.13%   90.12%   -0.02%     
==========================================
  Files         752      752              
  Lines      251568   251816     +248     
  Branches    47270    47348      +78     
==========================================
+ Hits       226759   226943     +184     
- Misses      16168    16176       +8     
- Partials     8641     8697      +56     
Files with missing lines Coverage Δ
src/node_platform.cc 74.75% <94.73%> (-1.25%) ⬇️
src/node_platform.h 85.71% <80.00%> (-5.96%) ⬇️

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

Comment thread src/node_platform.cc
Comment on lines +770 to +771
std::unique_ptr<T> task =
std::move(const_cast<Item&>(task_queue_.top()).task);

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.

Nit: The readability of this is... poor ;-) ... Might be better to split these lines up a bit.

@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 17, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants