queue: Add custom task queue support - #92
Conversation
Signed-off-by: Chao Wang <cclcwangchao@hotmail.com>
Signed-off-by: Chao Wang <cclcwangchao@hotmail.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds typed custom queue selection, a new custom task-queue abstraction, and ChangesCustom queue and PopResult rollout
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/queue/custom.rs (1)
42-47: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winClarify or guard repeated
draincalls for shared custom queues.
Builder::buildgives every local handle the same queue, and each handle forwardsdrain()directly. In multi-worker shutdown, custom implementations may see multiple/concurrent drain calls; either document thatTaskQueue::drainmust be idempotent/thread-safe, or guard the adapter so the shared queue is drained once.Also applies to: 100-110, 176-179
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/queue/custom.rs` around lines 42 - 47, `TaskQueue::drain` can be called multiple times or concurrently because `Builder::build` shares the same queue across local handles and each handle forwards `drain()` directly. Update the `TaskQueue`/adapter path to either clearly document that `TaskQueue::drain` must be idempotent and thread-safe, or add a guard so the shared queue is drained only once; focus on the `TaskQueue` trait and the local handle/shutdown forwarding code that invokes `drain()`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/task/future.rs`:
- Line 302: The thread-local initializer in LOCAL uses const block syntax that
requires Rust 1.79, so either add a rust-version declaration in Cargo.toml to
match that minimum toolchain or refactor the LOCAL initialization in future.rs
to avoid const { ... } if older compiler support must be preserved.
---
Nitpick comments:
In `@src/queue/custom.rs`:
- Around line 42-47: `TaskQueue::drain` can be called multiple times or
concurrently because `Builder::build` shares the same queue across local handles
and each handle forwards `drain()` directly. Update the `TaskQueue`/adapter path
to either clearly document that `TaskQueue::drain` must be idempotent and
thread-safe, or add a guard so the shared queue is drained only once; focus on
the `TaskQueue` trait and the local handle/shutdown forwarding code that invokes
`drain()`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: abce53b9-4521-4d5f-811b-edad27e464d2
📒 Files selected for processing (9)
src/pool/builder.rssrc/pool/spawn.rssrc/pool/worker.rssrc/queue.rssrc/queue/custom.rssrc/queue/multilevel.rssrc/queue/priority.rssrc/task/callback.rssrc/task/future.rs
|
/retest |
da431e7 to
994dc0f
Compare
Signed-off-by: Chao Wang <cclcwangchao@hotmail.com>
994dc0f to
40168c7
Compare
|
@coderabbitai review it again |
|
✅ Action performedFull review finished. |
|
Review Finding: |
Signed-off-by: Chao Wang <cclcwangchao@hotmail.com>
Signed-off-by: Chao Wang <cclcwangchao@hotmail.com>
Thanks for the detailed report. Fixed in a3f5419 by handling the When a worker above the current I chose this approach to keep the existing scale-down behavior for already-running workers unchanged, while fixing the new timed-Pending wakeup path introduced by custom queues. I also added failpoint tests covering:
|
Signed-off-by: Chao Wang <cclcwangchao@hotmail.com>
41a86d6 to
7dce82c
Compare
Signed-off-by: Chao Wang <cclcwangchao@hotmail.com>
Signed-off-by: Chao Wang <cclcwangchao@hotmail.com>
Signed-off-by: Chao Wang <cclcwangchao@hotmail.com>
Summary
This PR adds support for using a user-provided task queue in YATP.
queue::TaskQueue<T>andqueue::PopResult<T>so a queue can report whether work is ready, delayed, or absent.Builder::build_custom_future_pooland wires custom queues into the existing injector/local-queue plumbing.PopResult::Pending { retry_at }: a worker may sleep until the retry deadline, while a laterspawncan still wake it immediately.task::future::yield_to_scheduler()for future tasks that need to force the next poll through the scheduler queue instead of relying on the normal preemption hint.Custom Queue API
A custom queue implements
queue::TaskQueue<T>:The most important contract is
pop():PopResult::Ready(pop)means a task can run immediately.PopResult::Pending { retry_at }means the queue contains tasks, but none can run yet. The worker will retry no later thanretry_at, and can still be woken earlier by a later push.PopResult::Emptymeans the queue contains no tasks.Important: if the queue still owns delayed or throttled tasks, it should return
Pending, notEmpty. ReturningEmptycan let workers park as if no work exists, which may delay those tasks for a long time.Example
Suppose a custom queue delays tasks and keeps the earliest runnable task at the top of a heap. For brevity,
Scheduled<T>is ordered byready_at, and the heap storesReverse<Scheduled<T>>so the earliest time is popped first.pushrecords when the task entered the queue and when it can first run:popchecks the earliest scheduled task. If it is ready, the worker can run it now. If it is not ready, the queue returns the earliest retry deadline instead of pretending to be empty.The remaining trait methods can stay simple for this queue:
drain()clears the heap, andhas_ready_task()checks whethertasks.peek()is already ready without removing it.A custom future pool can then be built by passing the queue as
Arc<dyn TaskQueue<future::TaskCell>>:yield_to_schedulerYATP future tasks already have
task::future::reschedule(), which gives the scheduler a chance to run other ready work. However,reschedule()still follows the normal preemption check: if the scheduler does not report ready work, the current worker may keep polling the same task for locality.task::future::yield_to_scheduler()is stricter. It always returns the current future task to the scheduler queue before polling it again, skipping thehas_ready_task()/ preemption hint. This is useful when the queue itself should make the next scheduling decision, for example when a custom queue uses delayed readiness, rate limiting, or another policy that should be applied between task slices.Tests
cargo fmt --checkcargo clippy --tests -- -D clippy::allcargo clippy --tests --features failpoints -- -D clippy::allcargo test --testscargo test --tests --features failpointscargo test yield_to_schedulerSummary by CodeRabbit