src: run same-priority platform tasks in posting order - #65353
Merged
nodejs-github-bot merged 1 commit intoAug 20, 2026
Merged
Conversation
Copy link
Copy Markdown
Member
Sorry, something went wrong.
Copy link
Copy Markdown
Codecov Report❌ Patch coverage is 91.66667% with 2 lines in your changes missing coverage. Please review.
@@ Coverage Diff @@
## main #65353 +/- ##
==========================================
- Coverage 90.13% 90.12% -0.02%
==========================================
Files 752 752
Lines 251568 251857 +289
Branches 47270 47364 +94
==========================================
+ Hits 226759 226991 +232
- Misses 16168 16191 +23
- Partials 8641 8675 +34
... and 55 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Sorry, something went wrong.
jasnell
reviewed
Aug 17, 2026
Comment thread
src/node_platform.cc
Outdated
jasnell
approved these changes
Aug 17, 2026
Copy link
Copy Markdown
Collaborator
Sorry, something went wrong.
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
force-pushed
the
fix/platform-task-queue-fifo
branch
from
121482b to
25b1017
Compare
August 18, 2026 08:12
codebytere
requested review from
aduh95 and
anonrig
and removed request for
joyeecheung and
santigimeno
August 19, 2026 08:53
Copy link
Copy Markdown
Collaborator
Sorry, something went wrong.
anonrig
approved these changes
Aug 19, 2026
Copy link
Copy Markdown
Collaborator
Sorry, something went wrong.
Copy link
Copy Markdown
|
Sorry, something went wrong.
Copy link
Copy Markdown
Collaborator
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
|
When there is a high degree of concurrency, this modification to the pr might cause node.js to freeze. For example, when a large number of https requests are initiated concurrently. After rolling back this pr, the problem disappeared. I'm not sure about the exact reason. |
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
|
Reproducible script const CONCURRENCY = 64
const URL = 'https://qq.com'
async function main () {
console.log(`🚀 Firing ${CONCURRENCY} concurrent requests → ${URL}\n`)
const promises = Array.from({ length: CONCURRENCY }, async (_, i) => {
const start = Date.now()
try {
const res = await fetch(URL, { method: 'GET' })
await res.text()
console.log(`#${i + 1} ✓ status=${res.status} ${Date.now() - start}ms`)
return { ok: true, status: res.status }
} catch (err) {
console.log(`#${i + 1} ✗ error: ${err.message}`)
return { ok: false }
}
})
const results = await Promise.all(promises)
const ok = results.filter(r => r.ok).length
console.log(`\n✅ Done: ${ok}/${CONCURRENCY} succeeded`)
}
main() |
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
|
Sorry, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Reviewers
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.