Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add cancellation token to binaries #821

Merged
merged 14 commits into from
May 10, 2024
Merged

feat: add cancellation token to binaries #821

merged 14 commits into from
May 10, 2024

Conversation

carneiro-cw
Copy link
Contributor

No description provided.

@carneiro-cw carneiro-cw requested a review from a team as a code owner May 10, 2024 14:39
Copy link

PR Review 🔍

⏱️ Estimated effort to review [1-5]

3, because the PR involves multiple files and integrates cancellation tokens across different components which requires a good understanding of asynchronous programming and signal handling in Rust. The changes are spread across several files and involve critical functionality like signal handling and task cancellation which need careful review to ensure reliability and correctness.

🧪 Relevant tests

No

⚡ Possible issues

Possible Bug: The signal_handler function spawns a new task for listening to the CTRL+C signal and cancels the CancellationToken upon receiving the signal. However, there's no handling for the scenario where the signal listening fails immediately, which could leave the cancellation token in a non-cancelled state indefinitely if the error occurs.

🔒 Security concerns

No

Code feedback:
relevant filesrc/utils.rs
suggestion      

Consider implementing error handling for the spawned task in signal_handler to ensure that the system can respond appropriately to failures in setting up the signal listener. This could involve retrying the signal listening setup or propagating an error state that can be handled elsewhere in the application. [important]

relevant linetokio::spawn(async move {

Copy link

PR Code Suggestions ✨

CategorySuggestions                                                                                                                                                       
Enhancement
Improve error handling during signal handler setup.

Replace the direct call to signal_handler() with a more robust error handling mechanism to
ensure that the cancellation token is only created if the signal handler setup is
successful.

src/bin/importer_offline.rs [77]

-let cancellation = signal_handler();
+let cancellation = match signal_handler() {
+    Ok(token) => token,
+    Err(e) => {
+        tracing::error!("Failed to set up signal handler: {}", e);
+        return Err(anyhow::anyhow!("Failed to set up signal handler"));
+    }
+};
 
Enhance logging for better debugging and traceability.

Use structured logging for better traceability and debugging. Include more context in the
logging statements.

src/bin/importer_online.rs [69]

-tracing::info!("run_importer_online task cancelled, exiting");
+tracing::info!("run_importer_online task cancelled, exiting at block number: {}", number);
 
Best practice
Add cleanup or finalization steps before breaking out of the loop on cancellation.

Ensure that the CancellationToken is properly checked before proceeding with operations in
the loop to prevent unnecessary operations after cancellation.

src/bin/importer_online.rs [68-71]

 if cancellation.is_cancelled() {
     tracing::info!("run_importer_online task cancelled, exiting");
     break;
 }
+// Perform necessary cleanup or finalization here before breaking out of the loop
 
Error handling
Add error handling for the run_importer_online function.

Add error handling for the run_importer_online function to handle potential failures
gracefully.

src/bin/run_with_importer.rs [39]

-let importer_task = run_importer_online(executor, miner, storage, chain, cancellation);
+let importer_task = run_importer_online(executor, miner, storage, chain, cancellation)
+    .await
+    .map_err(|e| {
+        tracing::error!("Failed to run importer online: {}", e);
+        e
+    })?;
 
Modify signal_handler to handle errors in signal setup.

Modify the signal_handler function to return a Result<CancellationToken, Error> to handle
potential errors in signal setup.

src/utils.rs [8-21]

-pub fn signal_handler() -> CancellationToken {
+pub fn signal_handler() -> Result<CancellationToken, anyhow::Error> {
     let cancellation = CancellationToken::new();
     let task_cancellation = cancellation.clone();
     tokio::spawn(async move {
         match tokio::signal::ctrl_c().await {
             Ok(()) => {
                 tracing::info!("stop signal received, shutting down");
                 task_cancellation.cancel();
             }
-            Err(err) => tracing::error!("unable to listen for shutdown signal: {}", err),
+            Err(err) => {
+                tracing::error!("unable to listen for shutdown signal: {}", err);
+                return Err(anyhow::anyhow!("Signal setup failed: {}", err));
+            },
         }
     });
-    cancellation
+    Ok(cancellation)
 }
 

@carneiro-cw carneiro-cw merged commit 9356858 into main May 10, 2024
25 checks passed
@carneiro-cw carneiro-cw deleted the cancellation branch May 10, 2024 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant