-
Notifications
You must be signed in to change notification settings - Fork 44
fix: make Messenger::request
cancellation-safe
#115
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
Conversation
This avoids that we write partial messages to the underlying TCP/TLS connection and mess up message framing. This will nearly always lead to broker-side errors and the broker closes the connection. Fixes #103.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, I think the test has a race, but otherwise 💪
} | ||
|
||
#[tokio::test] | ||
async fn test_cancel_request() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this test would be easier to follow if the loops were broken up into separate functions, but it's fine as is also
src/messenger.rs
Outdated
// simulated broker, just reads messages and answers w/ "api versions" responses | ||
let handle_broker = tokio::spawn(async move { | ||
for correlation_id in 0.. { | ||
rx_back.read_message(1_000).await.unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could possibly check that the message is an ApiVersions request
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
2fdfca0
to
db79dba
Compare
This avoids that we write partial messages to the underlying TCP/TLS
connection and mess up message framing. This will nearly always lead to
broker-side errors and the broker closes the connection.
Fixes #103.