fix(spider-client): Make the gRPC client's request futures Send. - #404
Conversation
…futures in the retry helper
WalkthroughThe retry helpers now accept futures returned by synchronous closures. Job and resource-group gRPC operations acquire a client from a cloned connection pool within each retry attempt, while compile-time checks validate client and future sendability. ChangesgRPC retry migration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SpiderClient
participant call_with_retry
participant ConnectionPool
participant GrpcClient
SpiderClient->>call_with_retry: submit or invoke RPC
call_with_retry->>ConnectionPool: get_client() per attempt
ConnectionPool-->>GrpcClient: client
GrpcClient-->>call_with_retry: RPC result
call_with_retry-->>SpiderClient: result or retry
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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
🤖 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 `@components/spider-client/src/grpc/job.rs`:
- Line 125: Replace each per-attempt `let request = request;` binding in
`components/spider-client/src/grpc/job.rs` at lines 125, 154, 183, 214, and 243
with a clone, matching the existing handling in `submit_job`; no other retry
logic changes are needed.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d6b9cbc2-04c5-43ba-be0c-a2ea7fe28d26
📒 Files selected for processing (3)
components/spider-client/src/grpc/job.rscomponents/spider-client/src/grpc/resource_group.rscomponents/spider-utils/src/grpc/retry.rs
…/spider into client-future-send
Send.Send.
LinZhihao-723
left a comment
There was a problem hiding this comment.
Some extra polishing.
Directly modified the PR title and PR description.
Description
Since #396 routed the client's gRPC calls through the
spider-utilsretry helper, each call site passed anasyncclosure (AsyncFnMut) tocall_with_retry. The future such a closure returns is an anonymous, unnameable type, so there was no way to require it to beSend— and in practice it wasn't. As a result, aSpiderClientcall could not betokio::spawned on a multi-threaded runtime, which is the common way callers drive concurrent jobs.This PR makes every future returned by the
spider-clientpublic APISend, and adds compile-time assertions so the guarantee cannot silently regress.The change has three parts:
1.
spider-utilsretry helper — bound the returned future asSend.execute_with_retryandcall_with_retrynow takeGrpcCall: FnMut() -> FutureTypewithFutureType: Future<Output = ...> + Send, replacing the previousAsyncFnMut() -> Result<...>. Naming the future via an explicit type parameter is what lets us attach the+ Sendbound; anasyncclosure's future cannot be named, so it cannot be bounded. This is the mechanism behind the whole PR. The retry loop still reconstructs a fresh future per attempt — the closure is a future producer, not a single future — so retry semantics are unchanged. The helper's only callers are the twospider-clientfiles updated in this same PR, so no other code is affected.2.
spider-clientcall sites — plain closures returning owned futures. Each gRPC call now uses a plainmove ||closure that acquires a pooled client, builds the request, and returns an ownedasync move { ... }future, rather than anasyncclosure that borrows captured state. Request construction was additionally moved inside the closure for every call so each retry attempt mints its own owned request;Copyid fields (JobId,ResourceGroupId) are used directly, while owned fields (Vec<u8>,String) are cloned per attempt — the same per-attempt clone the previous code already performed, just relocated. This keeps the produced futures free of borrowed, non-Sendstate.3.
SpiderClient-level compile-time assertions.client.rsnow asserts, at compile time and zero runtime cost, thatSpiderClientandSpiderClientBuilderareSend + Sync, and that every public async method (submit_job,start_job,cancel_job,get_job_state,get_job_outputs,get_job_error,add_resource_group,verify_resource_group) returns aSendfuture. The type-level check alone does not cover the futures, so both are needed: a future that captures a non-Sendvalue across an.awaitwould break spawnability while the handle staysSend + Sync.Note on the retry unit tests: they switched their shared invocation counter from
Cell<usize>toAtomicUsize. This is a direct consequence of the newFutureType: Sendbound — the test closures capture the counter by shared reference across an.await, and&TisSendonly whenT: Sync.Cellis!Sync, so it no longer satisfies the bound;AtomicUsizeisSyncand is the minimal fix.Checklist
breaking change.
Validation performed
Send.Summary by CodeRabbit