fix(abci): invalid error returned when codec terminates #117
Conversation
WalkthroughThe changes in this pull request involve modifications to the Changes
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![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 and nitpick comments (3)
abci/src/server/codec.rs (2)
100-111: Fix incomplete documentation.The documentation comment for the error handling section appears to be incomplete. The last sentence "and forwards to" is hanging.
- /// and forwards to + /// and forwards them back to the ABCI client.
Line range hint
231-321: Consider adding tests for error handling scenarios.While the existing test covers message processing, consider adding tests for:
- Verifying that
send()returnsError::Cancelled()when the worker terminates- Testing various worker termination scenarios (client disconnect, errors, cancellation)
Example test structure:
#[tokio::test] async fn test_send_after_worker_termination() { let (request_tx, _) = mpsc::channel::<abci::Request>(1); let (response_tx, response_rx) = mpsc::channel::<abci::Response>(1); let cancel = CancellationToken::new(); // Set up codec let (client, server) = tokio::io::duplex(1024); let codec = tokio_util::codec::Framed::new(server, super::Coder {}); // Start worker let worker_cancel = cancel.clone(); let hdl = tokio::spawn(super::Codec::process_worker_queues( codec, request_tx, response_rx, worker_cancel, )); // Cancel worker cancel.cancel(); hdl.await.unwrap(); // Verify send returns Error::Cancelled match response_tx.blocking_send(create_test_response()) { Err(e) => assert_matches!(e, Error::Cancelled()), _ => panic!("Expected Error::Cancelled"), } }abci/tests/kvstore.rs (1)
68-71: LGTM: Proper error handling testThe changes correctly test the new error behavior where
Codecworker returnsError::Cancelled(). The added debug logging will help with test diagnostics.Consider adding a comment explaining why we expect
Error::Cancelled()here, to make the test's intention clearer:+ // We expect Error::Cancelled() because the server is terminated by the cancel token assert!(matches!( next_client, Err(tenderdash_abci::Error::Cancelled()) ));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
abci/src/server/codec.rs(2 hunks)abci/tests/kvstore.rs(1 hunks)
🔇 Additional comments (4)
abci/src/server/codec.rs (2)
Line range hint 112-163: LGTM! Robust error handling implementation.
The implementation shows excellent attention to detail:
- Proper backpressure handling by checking
request_txcapacity - Consistent error handling with cancellation
- Clear handling of connection termination cases
- Well-structured concurrent processing using
tokio::select!
164-167: LGTM! Improved error handling clarity.
The change from a generic error to Error::Cancelled() provides better context about why the send operation failed. The comment clearly explains that this occurs when the worker has either errored or been cancelled.
abci/tests/kvstore.rs (2)
66-66: LGTM: Proper variable naming
Removing the unused variable prefix _ is appropriate as the td variable is now explicitly used for cleanup.
79-80: LGTM: Proper resource cleanup
The explicit drop of the TenderdashDocker instance ensures proper cleanup of resources. The added empty line improves readability by separating the cleanup from the test assertions.
Issue being fixed or feature implemented
When the
Codecworker terminates (eg. client disconnected or some error occurred), it returns a genericError::Async,Detected by flaky Github Actions pipeline.
What was done?
Return
Error::Cancelled()instead ofError::AsyncHow Has This Been Tested?
GHA pipeline pass
Breaking Changes
None
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Bug Fixes
Tests