[ws client]: return last seen error in connect #338
Conversation
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.
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.
It's documented here, not sufficient?
There was a problem hiding this comment.
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.
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.
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?
…jsonrpsee into na-ws-client-fix-bad-err-msg
The connect error is ignored and always returns
NoAddressFoundwhich this commit fixes.Related to #337