-
Notifications
You must be signed in to change notification settings - Fork 170
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
[ws client]: return last seen error in connect #338
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.
(but please adjust the docs here)
ws-client/src/transport.rs
Outdated
let mut err = Err(WsHandshakeError::NoAddressFound); | ||
for sockaddr in &self.target.sockaddrs { | ||
match self.try_connect(*sockaddr, &connector).await { | ||
Ok(res) => return Ok(res), | ||
Err(e) => { | ||
log::debug!("Failed to connect to sockaddr: {:?} with err: {:?}", sockaddr, e); | ||
err = Err(e.into()); | ||
} | ||
} | ||
} | ||
Err(WsHandshakeError::NoAddressFound) | ||
err |
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 a bit of a weird behaviour imo, but given std::net::TcpStream::connect
does it, I guess it's fine we do it too. :/
An alternative could be to return immediately, at the first error, unless the error is NoAddressFound
?
Either way, we need to adjust the docs for WsHandshakeError
.
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 documented here, not sufficient?
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 we can remove ResolutionFailed
because it's not possible anymore because the IP addresses are "resolved" before this but the sockaddrs could still be empty.
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 documented here, not sufficient?
I read that as if it applied only to the WsHandshakeError::Connect
variant, but it applies to all variants actually, like if I give 3 IPs, first fails with CertificateStore
, second with NoAddressFound
and the third Connect
, the caller would only see Connect
, correct? So maybe the docs should move to the enum itself? Not sure.
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.
fine, I merged to WsNewError
with WsHandshakeError
and added docs for the entire enum WsHandshakeError
it's technically not only WsHandshakeError
in there such as bad url
but better at least.
also improved the error messages a bit, I think this is good enough for now?
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
…jsonrpsee into na-ws-client-fix-bad-err-msg
The connect error is ignored and always returns
NoAddressFound
which this commit fixes.Related to #337