pool: fix scaled-down threads still processing tasks - #90
Conversation
When threads are scaled down, threads with ID > core_thread_count should go to sleep immediately. Previously, the park validation callback would still pop tasks from the queue, causing scaled-down threads to keep working instead of sleeping. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: rishabh mittal <rishabh.mittal@airbnb.com>
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/pool/spawn.rs (1)
332-336: Add a regression test for this scale-down edge case.Please add a test that scales down while tasks are pending and verifies workers above
core_thread_countdo not dequeue until scaled up (or shutdown). This path is concurrency-sensitive and worth pinning with coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pool/spawn.rs` around lines 332 - 336, Add a concurrency regression test that reproduces the scale-down edge case in spawn.rs: create a pool with more workers than core_thread_count, submit several long-blocking tasks (use a Barrier or channel to hold tasks), trigger a scale-down so some worker threads have id > core_thread_count, then assert those higher-id workers do not dequeue new tasks while scaled down by verifying the pending task queue size or an Atomic counter remains unchanged; finally scale up or shutdown to release blocked tasks and verify completion. Use synchronization primitives (Barrier/oneshot channels/AtomicUsize) to deterministically control task start/finish and reference the core_thread_count and id check in spawn.rs when locating the worker loop to ensure the test pins this path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/pool/spawn.rs`:
- Around line 332-336: Add a concurrency regression test that reproduces the
scale-down edge case in spawn.rs: create a pool with more workers than
core_thread_count, submit several long-blocking tasks (use a Barrier or channel
to hold tasks), trigger a scale-down so some worker threads have id >
core_thread_count, then assert those higher-id workers do not dequeue new tasks
while scaled down by verifying the pending task queue size or an Atomic counter
remains unchanged; finally scale up or shutdown to release blocked tasks and
verify completion. Use synchronization primitives (Barrier/oneshot
channels/AtomicUsize) to deterministically control task start/finish and
reference the core_thread_count and id check in spawn.rs when locating the
worker loop to ensure the test pins this path.
|
/retest |
1 similar comment
|
/retest |
| } | ||
| // If this thread is above core_thread_count, go to sleep | ||
| // without popping so scaled-down threads don't keep working. | ||
| if id > self.core.config.core_thread_count.load(Ordering::SeqCst) { |
There was a problem hiding this comment.
Can you explain the requirement that need to let the thread park immediately instead of trying to drain the tasks in the local queue? I think in most scenario, the difference should be very small.
There was a problem hiding this comment.
We use the TiKV thread pool auto-scaling feature and observed that under load, TiKV scales down the number of threads — but the yatp pool does not shrink because its threads are occupied draining the queue and unified read pool CPU remains higher than the number of active threads shown by TiKV
3 participants
Issue #19498
When threads are scaled down, threads with ID > core_thread_count should go to sleep immediately. Previously, the park validation callback would still pop tasks from the queue, causing scaled-down threads to keep working instead of sleeping.
Summary by CodeRabbit