Conversation
Extract the connection logic into `do_connect_peer_internal` and have `do_connect_peer` act as a thin wrapper that always calls `propagate_result_to_subscribers` with the result. This removes the need to manually propagate at every error site, making the code less error-prone. Co-Authored-By: HAL 9000
Replace the synchronous, blocking `std::net::ToSocketAddrs::to_socket_addrs()` calls with async `tokio::net::lookup_host` to avoid blocking the tokio runtime during DNS resolution. Additionally, instead of only using the first resolved address, we now iterate over all resolved addresses and try connecting to each in sequence until one succeeds. This improves connectivity for hostnames that resolve to multiple addresses (e.g., dual-stack IPv4/IPv6). Co-Authored-By: HAL 9000
|
👋 Thanks for assigning @tankyleo as a reviewer! |
| proxy_addr, | ||
| Arc::clone(&self.keys_manager), | ||
| ); | ||
| res = self.await_connection(connection_future, node_id, addr.clone()).await; |
There was a problem hiding this comment.
If res.is_err() here, we don't know whether we failed to connect to the proxy, or we failed to connect to the peer. Do you think this is worth fixing on the tor_connect_outbound API ?
If we failed to connect to the peer, but nonetheless successfully connected to the proxy, we'd try connecting again using a new resolved address for the proxy, even though the previous one worked fine.
I can see how this is intended behavior, as we could make a successful connection to the peer upon trying again with a different proxy address.
There was a problem hiding this comment.
If res.is_err() here, we don't know whether we failed to connect to the proxy, or we failed to connect to the peer. Do you think this is worth fixing on the tor_connect_outbound API ?
Not sure it's entirely worth discerning? We generally might want to simply retry, and maybe it's even preferable to do the whole handshake from scratch, i.e., including using a new stream for example?
Co-Authored-By: HAL 9000
Co-Authored-By: HAL 9000
tankyleo
left a comment
tankyleo
left a comment
There was a problem hiding this comment.
LGTM feel free to squash
| had_failures = true; | ||
| log_debug!( | ||
| self.logger, | ||
| "Failed to connect to peer {}@{} via resolved proxy address {}, trying next address.", |
There was a problem hiding this comment.
nit: we won't be trying a next address if we are at the end of the iteration, here and below
3 participants
Previously, we used
std::net::ToSocketAddrsfor DNS resolution. While this generally worked, we also used the blockingto_socket_addrscall in async contexts, blocking a runtime task during resolution. This isn't great, so here we switch to usetokio::net::lookup_hostfor async resolution.