-
Notifications
You must be signed in to change notification settings - Fork 25
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
Disconnection test #76
Conversation
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.
Let's also add a test that uses the connection pool to create a connection to the server and then that connection fails. (This need not be an integration test, you can mock the server)
low_version: 5, | ||
}); | ||
let reply = connection.write(&request).await; | ||
// TODO: Tests failed here. It will always gives BrokenPipe error. |
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.
This looks like a problem with the pool. If you don't have any ideas, just as a test rry running on top of this PR #75
which changes how the pool works and see if anything changes.
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.
fixed.
wire_protocol/src/connection_pool.rs
Outdated
}), | ||
Ok(mut conn) => { | ||
// The connection may be closed by the server when it is in the pool. | ||
if conn.is_valid().await { |
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 guess we can somehow use a loop to check every conn in the managed_pool until there is a valid one or there is nothing left. Right now it just checks one conn at most.
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.
fixed.
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.
Few comments. Otherwise looks good.
async fn test_retry_with_no_connection() { | ||
let retry_policy = RetryWithBackoff::default().max_tries(4); | ||
// give a wrong endpoint | ||
let endpoint = "127.0.0.1:12345" |
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.
It's possible for something to actually be on that port, in which case this would fail in an unexpected way.
https://en.wikipedia.org/wiki/List_of_TCP_and_UDP_port_numbers#Well-known_ports
Maybe something like 2, would be better? Does 0 work? Because that definitivly can't have anything listening on it.
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.
Yes, I change the port to 0.
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.
LGTM...
Change log description
Implement the disconnection test.
Purpose of the change
Fixes: #70 #81
How to verify it
tests should pass.