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

Handling errors in RPC Tokio tasks #502

Open
4 tasks
ceyhunsen opened this issue Feb 5, 2025 · 4 comments
Open
4 tasks

Handling errors in RPC Tokio tasks #502

ceyhunsen opened this issue Feb 5, 2025 · 4 comments
Assignees
Labels
bug Something isn't working code quality Improves code quality while not affecting any other P-Low Priority: Low question Further information is requested

Comments

@ceyhunsen
Copy link
Member

ceyhunsen commented Feb 5, 2025

Issue Description

Tokio tasks used in some RPC functions returns Result. But the handle of those tasks are never checked for errors after they finish. Possible errors are only related to Tokio channels and these errors can be discovered while testing. But because these errors are silent, developer might have a hard time finding the errors.

Example code:

tokio::spawn(async move {
// First send the session id
let session_id: NonceGenResponse = nonce_gen_first_response.into();
tx.send(Ok(session_id)).await?;
// Then send the public nonces
for pub_nonce in &pub_nonces[..] {
let pub_nonce: NonceGenResponse = pub_nonce.into();
tx.send(Ok(pub_nonce)).await?;
}
Ok::<(), SendError<_>>(())
});

There are multiple possible solutions to this.

Possible Solutions

  • Convert ?'s to except() calls
  • Write a task watcher background job that accepts task handles and logs them if anyone returns an Err
  • Create a macro that spawns another infallible Tokio task that logs the main task's errors, if there are any
  • Move a task's contents to a function with a #[tracing::instrument] so that tracing handles this
@ceyhunsen ceyhunsen added bug Something isn't working code quality Improves code quality while not affecting any other P-Low Priority: Low question Further information is requested labels Feb 5, 2025
@mmtftr
Copy link

mmtftr commented Feb 5, 2025

I've actually done solution #2 in some places when I was refactoring. I think it makes sense to go with 2 and maybe write a helper macro/function for it. Or we could write the inner as a function somewhere and add #[tracing::instrument] to it

@ceyhunsen
Copy link
Member Author

@mmtftr I think you mentioned a wrong PR. Can you also edit the description and add the solution you provided as an option?

@mmtftr
Copy link

mmtftr commented Feb 5, 2025

Yeah, didn't mean to mention a PR. I was referring to #(number)2. You can check

// Background task to handle the error case where the background task fails, notifies caller
tokio::spawn(async move {
if let Ok(Err(bg_err)) = handle.await {
let ret_res = error_tx.send(Err(bg_err)).await;
if let Err(SendError(Err(e))) = ret_res {
tracing::error!("deposit_sign background task failed and the return stream ended prematurely:\n\n Background task error: {e}");
}
}
});
for an example

@ceyhunsen
Copy link
Member Author

OK, thanks. Added them as options

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working code quality Improves code quality while not affecting any other P-Low Priority: Low question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants