fix: wrap more expensive calls with spawn_blocking#7236
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughCPU-intensive state simulation and circulating-supply computation now run through async wrappers backed by ChangesAsync spawn_blocking refactor for state ops and crypto
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
spawn_blockingspawn_blocking
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/rpc/methods/miner.rs (1)
246-250: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd context to blocking-task join failures.
Line 249 and Line 275 return
JoinErrorvia?without task-specific context. Add.context(...)on each.awaitso RPC failures are diagnosable.As per coding guidelines, "
**/*.rs: Use anyhow::Result for most operations and add context with.context()when errors occur".Suggested patch
async fn sign_block_header( block_header: Arc<RawBlockHeader>, worker: Address, keystore: Arc<RwLock<KeyStore>>, ) -> Result<Signature> { tokio::task::spawn_blocking(move || { sign_block_header_blocking(&block_header, &worker, &keystore) }) - .await? + .await + .context("failed to join blocking task for block-header signing")? } @@ async fn aggregate_from_bls_signatures(bls_sigs: Vec<Signature>) -> anyhow::Result<Signature> { - tokio::task::spawn_blocking(move || aggregate_from_bls_signatures_blocking(bls_sigs)).await? + tokio::task::spawn_blocking(move || aggregate_from_bls_signatures_blocking(bls_sigs)) + .await + .context("failed to join blocking task for BLS signature aggregation")? }Also applies to: 275-276
🤖 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/rpc/methods/miner.rs` around lines 246 - 250, Add task-specific context to the blocking-task join failures in the RPC methods by updating the `tokio::task::spawn_blocking(...).await?` calls in the affected miner signing flow to use `.context(...)` before propagating the error. Make the context message specific to the operation in `sign_block_header_blocking` and the other matching blocking-task await in this file so any `JoinError` clearly identifies which RPC step failed.Source: Coding guidelines
🤖 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/state_manager/circulating_supply.rs`:
- Around line 138-140: The error-string shortening in circulating_supply.rs is
truncating a String at a fixed byte offset, which can panic on UTF-8 boundaries.
Update the logic around the e.to_string() and e.truncate(100) path to shorten
only at a valid character boundary, either by clamping to the nearest char
boundary before truncation or by using a boundary-aware helper, while keeping
the bounded error size behavior.
---
Nitpick comments:
In `@src/rpc/methods/miner.rs`:
- Around line 246-250: Add task-specific context to the blocking-task join
failures in the RPC methods by updating the
`tokio::task::spawn_blocking(...).await?` calls in the affected miner signing
flow to use `.context(...)` before propagating the error. Make the context
message specific to the operation in `sign_block_header_blocking` and the other
matching blocking-task await in this file so any `JoinError` clearly identifies
which RPC step failed.
🪄 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: 8baae7be-acce-46d8-9857-0ac797944b3a
📒 Files selected for processing (5)
src/rpc/methods/eth.rssrc/rpc/methods/miner.rssrc/rpc/methods/state.rssrc/state_manager/circulating_supply.rssrc/state_manager/message_simulation.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
filecoin-project/lotus(manual)
Codecov Report❌ Patch coverage is Additional details and impacted files
... and 10 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
…e-fn-with-spawn-blocking
Summary of changes
Follow-up of #7184
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
New Features
Bug Fixes