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.
| Arc::clone(&self.keys_manager), | ||
| ); | ||
| res = self.await_connection(connection_future, node_id, addr.clone()).await; | ||
| if res.is_ok() { |
There was a problem hiding this comment.
In the err case, do we want to add a trace / debug log here to give a small heads up "hey we failed here" ? If we eventually succeed on a subsequent iteration, the transient error would go unnoticed.
| socket_addr, | ||
| ); | ||
| res = self.await_connection(connection_future, node_id, addr.clone()).await; | ||
| if res.is_ok() { |
There was a problem hiding this comment.
Similar to above, do we want to be more verbose in the logs here ? ie "hey this IP address failed, but eventually succeeded with this other IP address".
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.