Refactor(miner): replace block_on with async spawn + timeout + atomic… #4445
Conversation
WalkthroughThis pull request involves minor formatting changes in the Changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
… flag Avoid potential deadlock by removing blocking block_on. Use ctx.spawn + timeout + Arc<AtomicBool> to ensure only one task runs at a time. Preserves original minting logic while improving reliability.
sanlee42
force-pushed
the
refactor_miner
branch
from
03ad4f7 to
55793e1
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
miner/src/lib.rs (1)
300-308: 🛠️ Refactor suggestionConsider refactoring dispatch_sleep_task as well
The
dispatch_sleep_taskmethod still usesblock_on, which contradicts the goal of eliminating blocking operations that could cause deadlocks. Consider refactoring this method similarly todispatch_task.-pub fn dispatch_sleep_task(&mut self, ctx: &mut ServiceContext<Self>) -> Result<()> { - //create block template should block_on for avoid mint same block template. - let response = block_on(async { - self.create_block_template_service - .send(BlockTemplateRequest) - .await? - })?; - self.dispatch_mint_block_event(ctx, response.template) +pub fn dispatch_sleep_task(&mut self, ctx: &mut ServiceContext<Self>) -> Result<()> { + if self.task_flag.load(Ordering::Relaxed) { + debug!("Mint task already running, skip dispatch_sleep_task"); + return Ok(()); + } + self.task_flag.store(true, Ordering::Relaxed); + + let create_block_template_service = self.create_block_template_service.clone(); + let addr = ctx.service_ref::<MinerService>()?.clone(); + let flag = self.task_flag.clone(); + + ctx.spawn(async move { + let result = tokio::time::timeout( + Duration::from_millis(2000), + create_block_template_service.send(BlockTemplateRequest), + ).await; + + match result { + Ok(Ok(Ok(response))) => { + let _ = addr.send(DispatchMintBlockTemplate { + block_template: response.template + }).await; + }, + _ => { + warn!("Failed to get block template in dispatch_sleep_task"); + } + } + + flag.store(false, Ordering::Relaxed); + }); + + Ok(()) }
🧹 Nitpick comments (3)
miner/src/lib.rs (3)
133-135: Consider reducing log frequencyWhile it's good to have service health logs, emitting "MinerService is alive" every second might generate excessive logs in production. Consider increasing the interval to 1 minute or using a more detailed heartbeat message with metrics.
-ctx.run_interval(Duration::from_secs(1), |_ctx| { - info!("MinerService is alive"); +ctx.run_interval(Duration::from_secs(60), |_ctx| { + info!("MinerService heartbeat - tasks in pool: {}", self.task_pool.len());
141-141: Fix typo in log messageThere's a small typo in the log message.
-info!("stoped miner_serive "); +info!("stopped miner_service");
363-367: Clean up TODO commentThis TODO comment mentions "@SANLEE" who appears to be the PR author. Since the refactoring is in progress, consider updating or removing this comment.
-// TODO:: Refactor TaskMisMatchError,remove current @sanlee +// TODO: Refactor TaskMisMatchError to provide more detailed mismatch information
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
miner/Cargo.toml(1 hunks)miner/src/lib.rs(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Generate code coverage report
🔇 Additional comments (9)
miner/Cargo.toml (1)
33-33: Good addition of Actix dependencyThis addition of the Actix actor framework aligns with the PR objectives to replace
block_onwith asynchronous operations. Actix provides a solid foundation for building robust concurrent systems in Rust.miner/src/lib.rs (8)
28-28: Appropriate use of atomic primitivesAdding the atomic imports is necessary for the concurrent task management approach described in the PR objectives. This is a good practice for thread-safe flag management.
55-55: Good addition of atomic flag for task coordinationThe
task_flagfield usingArc<AtomicBool>is a good approach to safely coordinate concurrent tasks across multiple threads. This enables the non-blocking behavior mentioned in the PR objectives.
125-125: Clean initialization of the atomic flagProper initialization of the atomic flag in the constructor. This ensures the service starts in a non-running state.
163-201: Well-structured async block template handlingThe implementation of
SyncBlockTemplateRequestis well-structured and properly handles asynchronous block template creation. The approach avoids blocking operations and ensures proper error handling.A few observations:
- The code correctly uses oneshot channels for communication
- The spawned task ensures async execution without blocking the service
- Block template validation logic is correctly preserved
203-216: Clean implementation of dispatch handlerThe
DispatchMintBlockTemplatehandler is clean and focused, delegating to the existingdispatch_mint_block_eventmethod. This maintains good separation of concerns.
218-233: Good implementation of delayed event dispatchThe
DelayGenerateBlockEventimplementation correctly usesrun_laterto schedule a delayed notification, which helps with handling retry logic in a clean way.
241-244: Good use of atomic flag to prevent concurrent task executionThis check using the atomic flag is a key improvement mentioned in the PR objectives. It prevents multiple concurrent minting tasks from running simultaneously, which could lead to race conditions or resource contention.
247-295: Excellent refactoring of dispatch_task for async handlingThis is the core of the PR's improvement. The implementation:
- Properly replaces blocking operations with async equivalents
- Adds timeout handling to prevent indefinite waits
- Includes comprehensive error handling for all failure scenarios
- Ensures proper cleanup of the atomic flag in all code paths
- Implements a retry mechanism with delay for failed attempts
This implementation aligns perfectly with the PR objectives of replacing
block_onwith async spawn + timeout + atomic flag management.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/docker_build.yml (1)
61-61: Commented-out kubectl Patch Command Clarification
Thekubectl patchcommand is commented out on purpose. If this disablement is temporary (e.g., for debugging purposes), please consider adding a TODO or a more detailed comment explaining the rationale and under what conditions it should be re-enabled or removed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/docker_build.yml(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build and test
🔇 Additional comments (1)
.github/workflows/docker_build.yml (1)
52-53: Update Deployment Branch Condition for Clarity
The comment and the conditional now clearly restrict the deployment step to thedag-masterbranch. This aligns well with the PR objective of clarifying deployment on the correct branch.
Benchmark for 81f9c53Click to view benchmark
|
miner/Cargo.toml
Outdated
| @@ -30,7 +30,7 @@ tokio = { features = ["full"], workspace = true } | |||
| starcoin-dag = { workspace = true } | |||
| starcoin-types = { workspace = true } | |||
| starcoin-sync = { workspace = true } | |||
|
|
|||
| actix = {workspace =true} | |||
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
miner/src/lib.rs (2)
297-305: 🛠️ Refactor suggestionConsider refactoring dispatch_sleep_task as well
While
dispatch_taskhas been refactored to use asynchronous patterns,dispatch_sleep_taskstill usesblock_on. For consistency and to avoid potential deadlocks, consider refactoring this method to follow the same asynchronous pattern.- pub fn dispatch_sleep_task(&mut self, ctx: &mut ServiceContext<Self>) -> Result<()> { - //create block template should block_on for avoid mint same block template. - let response = block_on(async { - self.create_block_template_service - .send(BlockTemplateRequest) - .await? - })?; - self.dispatch_mint_block_event(ctx, response.template) - } + pub fn dispatch_sleep_task(&mut self, ctx: &mut ServiceContext<Self>) -> Result<()> { + if self.task_flag.load(Ordering::Relaxed) { + debug!("Mint task already running, skip sleep dispatch"); + return Ok(()); + } + self.task_flag.store(true, Ordering::Relaxed); + + let create_block_template_service = self.create_block_template_service.clone(); + let addr = ctx.service_ref::<MinerService>()?.clone(); + let flag = self.task_flag.clone(); + + ctx.spawn(async move { + match create_block_template_service.send(BlockTemplateRequest).await { + Ok(Ok(response)) => { + let _ = addr + .send(DispatchMintBlockTemplate { block_template: response.template }) + .await; + } + _ => { + warn!("Failed to get block template in dispatch_sleep_task"); + } + } + flag.store(false, Ordering::Relaxed); + }); + + Ok(()) + }
360-364: 💡 Verification agent🧩 Analysis chain
Consider implementing the suggested TODO for MinerError
There's a TODO comment that mentions refactoring the
TaskMisMatchError. Since you're already refactoring this file, it might be a good opportunity to address this.
🌐 Web query:
What's the best practice for error handling in Rust when comparing expected vs actual values?💡 Result:
To handle errors effectively when comparing expected vs actual values in Rust, follow these best practices:
Use Structured Error Handling
Leverage
ResultandOptiontypes for explicit error handling instead of panics:// Return Result<T, E> for recoverable errors fn compare_values<T: PartialEq>(a: T, b: T) -> Result<(), ComparisonError> { if a == b { Ok(()) } else { Err(ComparisonError::Mismatch) } }Avoid
unwrap()/expect()in production code [3][7].Implement custom error types with
thiserrorfor clarity:#[derive(Debug, thiserror::Error)] enum ComparisonError { #[error("Value mismatch: expected {expected}, got {actual}")] Mismatch { expected: String, actual: String }, }[1][10][17]
Testing and Assertions
Use
assert_eq!with standardized ordering:assert_eq!(actual_value, expected_value); // Preferred order: actual then expectedThe panic message shows
left(actual) vsright(expected) [5][9].For complex comparisons, use specialized crates:
assert_matches!for pattern matching:assert_matches!(result, Ok(_));assert_json_difffor structured JSON comparisons [16]Handle
Optioncomparisons safely:assert!(result.is_none()); // For None checks assert_matches!(result, Some(42)); // Pattern matchingAvoid
assert_eq!when types lackPartialEq[11].Error Propagation and Context
Use
?operator for concise error propagation:let expected = parse_input()?; let actual = compute_output()?;[3][13]
Add context to errors using
.map_err()oranyhow:read_file() .map_err(|e| ComparisonError::IoError { source: e })?;[7][17]
Comparison of Approaches
Scenario Recommended Approach Avoid Simple equality checks assert_eq!(actual, expected)unwrap()comparisonsComplex struct matching assert_matches!Raw matchstatementsJSON value comparisons assert_json_diff::assert_json_eqManual field checks Option/Result unpacking assert!(result.is_some())assert_eq!(result, Some(...))withoutPartialEqDiagnostic Best Practices
- Implement
Debugfor all error and value types to enable meaningful test failure messages [11]- Use
expect-testcrate for self-updating snapshot tests [8]- Prefer
assert!with diagnostics over unwrapping:[5][16]assert!( actual == expected, "Mismatch: actual={:?}, expected={:?}", actual, expected );By following these patterns, you create maintainable error handling that provides clear diagnostics while adhering to Rust's type-safe error management philosophy.
Citations:
- 1: https://dev.to/nathan20/how-to-handle-errors-in-rust-a-comprehensive-guide-1cco
- 2: https://helpercode.com/2016/03/21/comparing-two-objects-using-assert-areequal/
- 3: https://www.ceos3c.com/rust/rust-error-handling-a-complete-guide/
- 4: https://doc.rust-lang.org/book/ch09-00-error-handling.html
- 5: https://users.rust-lang.org/t/assert-eq-expected-and-actual/20304
- 6: https://users.rust-lang.org/t/handling-rust-errors-concisely-consistently/13354
- 7: https://blog.logrocket.com/error-handling-rust/
- 8: https://docs.rs/expect-test/latest/expect_test/
- 9: https://www.ianlewis.org/en/rust-first-impressions-error-handling
- 10: https://greptime.com/blogs/2024-05-07-error-rust
- 11: https://users.rust-lang.org/t/no-option-none-comparison-unwrapping-with-embedded-generics/46273
- 12: https://users.rust-lang.org/t/did-rust-make-the-right-choice-about-error-handling/41736
- 13: https://bitfieldconsulting.com/posts/rust-errors-option-result
- 14: https://users.rust-lang.org/t/how-is-the-principle-in-rust-to-compare-two-f32-numbers/100499
- 15: https://www.youtube.com/watch?v=j-VQCYP7wyw
- 16: https://docs.rs/assert-json-diff
- 17: https://dev-state.com/posts/error_handling/
- 18: https://users.rust-lang.org/t/using-reference-to-do-comparison-instead-of-value/43074
- 19: https://users.rust-lang.org/t/best-practices-for-handling-io-errors-in-libraries/115945
- 20: Show values of expected and actual values for comparisons r-lib/testthat#104
Action Required: Refactor TaskMisMatchError for Structured Error Handling
The TODO in
miner/src/lib.rs(lines 360–364) calls for a refactor ofTaskMisMatchError. Given best practices in Rust for comparing expected versus actual values, please update the error to use structured error handling. Specifically:
Define a Custom Error Variant:
Instead of simply converting values to strings, create a variant that explicitly contains both the expected and actual values. For example, consider usingthiserrorfor clearer error messages:#[derive(Debug, thiserror::Error)] pub enum MinerError { #[error("Task mismatch: expected {expected}, got {actual}")] TaskMisMatchError { expected: String, actual: String }, // other variants... }Provide Meaningful Comparisons:
Leverage Rust’sResultandOptiontypes for error handling. This ensures that when comparing expected vs. actual values, the error message provides clear diagnostics, avoiding generic error strings.Please update the corresponding code in
miner/src/lib.rsaccordingly.
🧹 Nitpick comments (1)
miner/src/lib.rs (1)
138-138: Minor typo in log messageThere's a small typo in the log message: "stoped" should be "stopped".
- info!("stoped miner_serive "); + info!("stopped miner_service");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
miner/Cargo.toml(0 hunks)miner/src/lib.rs(5 hunks)
💤 Files with no reviewable changes (1)
- miner/Cargo.toml
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Generate code coverage report
🔇 Additional comments (7)
miner/src/lib.rs (7)
28-28: New atomic flag for concurrent task managementThe introduction of
AtomicBooland addition oftask_flagfield to theMinerServicestruct are good changes that will help prevent concurrent task execution issues.Also applies to: 55-55
125-125: Field initialization looks correctThe
task_flagis properly initialized tofalsewith atomic sharing capabilities.
160-198: Good addition of SyncBlockTemplateRequest handlerThis new handler properly implements the asynchronous template request pattern, which will help prevent blocking operations. The implementation is thorough with good error handling.
200-213: Well-structured DispatchMintBlockTemplate handlerThis handler provides a clean separation of concerns, allowing for dedicated handling of template dispatching.
215-230: Good implementation of delayed event handlingThe
DelayGenerateBlockEventhandler provides a nice pattern for scheduling future events, which improves the overall system reliability.
237-248: Effective task flag implementationThe check for an already running task and the setting of the flag are implemented correctly. Using
Ordering::Relaxedis appropriate for this use case since precise ordering isn't required.
249-293: Excellent refactoring of dispatch_task with timeout handlingThe replacement of
block_onwith an asynchronous approach using timeout is a significant improvement. The code now properly handles timeout scenarios and error cases, retrying after appropriate delays. The atomic flag is correctly reset at the end of the task.
Benchmark for b11000dClick to view benchmark
|
… flag
Avoid potential deadlock by removing blocking block_on. Use ctx.spawn + timeout + Arc to ensure only one task runs at a time.
Preserves original minting logic while improving reliability.
Pull request type
Please check the type of change your PR introduces:
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Other information
Summary by CodeRabbit
SyncBlockTemplateRequest,DispatchMintBlockTemplate, andDelayGenerateBlockEvent.