-
Notifications
You must be signed in to change notification settings - Fork 255
Add connect timeout #6110
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
base: develop
Are you sure you want to change the base?
Add connect timeout #6110
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -65,6 +65,7 @@ use std::fmt::Debug; | |
| use std::os::raw::c_int as RawFd; | ||
| use std::path::Path; | ||
| use std::sync::Arc; | ||
| use std::time::Duration; | ||
| use time::OffsetDateTime; | ||
| use tokio::sync::mpsc::Sender; | ||
| use url::Url; | ||
|
|
@@ -230,6 +231,7 @@ pub struct BaseClientBuilder<C, S: MixnetClientStorage> { | |
|
|
||
| #[cfg(unix)] | ||
| connection_fd_callback: Option<Arc<dyn Fn(RawFd) + Send + Sync>>, | ||
| connect_timeout: Option<Duration>, | ||
|
|
||
| derivation_material: Option<DerivationMaterial>, | ||
| } | ||
|
|
@@ -258,6 +260,7 @@ where | |
| setup_method: GatewaySetup::MustLoad { gateway_id: None }, | ||
| #[cfg(unix)] | ||
| connection_fd_callback: None, | ||
| connect_timeout: None, | ||
| derivation_material: None, | ||
| } | ||
| } | ||
|
|
@@ -356,6 +359,11 @@ where | |
| self | ||
| } | ||
|
|
||
| pub fn with_connect_timeout(mut self, timeout: Duration) -> Self { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it would it be easier for you if rather than setting connection timeout as a separate thing of the client, you put it as part of the base client config. |
||
| self.connect_timeout = Some(timeout); | ||
| self | ||
| } | ||
|
|
||
| // note: do **NOT** make this method public as its only valid usage is from within `start_base` | ||
| // because it relies on the crypto keys being already loaded | ||
| fn mix_address(details: &InitialisationResult) -> Recipient { | ||
|
|
@@ -533,6 +541,7 @@ where | |
| packet_router: PacketRouter, | ||
| stats_reporter: ClientStatsSender, | ||
| #[cfg(unix)] connection_fd_callback: Option<Arc<dyn Fn(RawFd) + Send + Sync>>, | ||
| connect_timeout: Option<Duration>, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and then I guess (with my previous comment in mind), all of those extra changes could be avoided |
||
| shutdown_tracker: &ShutdownTracker, | ||
| ) -> Result<GatewayClient<C, S::CredentialStore>, ClientCoreError> | ||
| where | ||
|
|
@@ -577,6 +586,7 @@ where | |
| stats_reporter, | ||
| #[cfg(unix)] | ||
| connection_fd_callback, | ||
| connect_timeout, | ||
| shutdown_tracker.clone_shutdown_token(), | ||
| ) | ||
| }; | ||
|
|
@@ -640,6 +650,7 @@ where | |
| packet_router: PacketRouter, | ||
| stats_reporter: ClientStatsSender, | ||
| #[cfg(unix)] connection_fd_callback: Option<Arc<dyn Fn(RawFd) + Send + Sync>>, | ||
| connect_timeout: Option<Duration>, | ||
| shutdown_tracker: &ShutdownTracker, | ||
| ) -> Result<Box<dyn GatewayTransceiver + Send>, ClientCoreError> | ||
| where | ||
|
|
@@ -672,6 +683,7 @@ where | |
| stats_reporter, | ||
| #[cfg(unix)] | ||
| connection_fd_callback, | ||
| connect_timeout, | ||
| shutdown_tracker, | ||
| ) | ||
| .await?; | ||
|
|
@@ -1074,6 +1086,7 @@ where | |
| stats_reporter.clone(), | ||
| #[cfg(unix)] | ||
| self.connection_fd_callback, | ||
| self.connect_timeout, | ||
| &shutdown_tracker.child_tracker(), | ||
| ) | ||
| .await?; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| use crate::error::GatewayClientError; | ||
|
|
||
| use nym_http_api_client::HickoryDnsResolver; | ||
| use std::time::Duration; | ||
| #[cfg(unix)] | ||
| use std::{ | ||
| os::fd::{AsRawFd, RawFd}, | ||
|
|
@@ -17,6 +18,7 @@ use std::net::SocketAddr; | |
| pub(crate) async fn connect_async( | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. while going through the code, I've noticed we have identical |
||
| endpoint: &str, | ||
| #[cfg(unix)] connection_fd_callback: Option<Arc<dyn Fn(RawFd) + Send + Sync>>, | ||
| connect_timeout: Option<Duration>, | ||
| ) -> Result<(WebSocketStream<MaybeTlsStream<TcpStream>>, Response), GatewayClientError> { | ||
| use tokio::net::TcpSocket; | ||
|
|
||
|
|
@@ -64,7 +66,22 @@ pub(crate) async fn connect_async( | |
| callback.as_ref()(socket.as_raw_fd()); | ||
| } | ||
|
|
||
| match socket.connect(sock_addr).await { | ||
| let connect_res = if let Some(connect_timeout) = connect_timeout { | ||
| match tokio::time::timeout(connect_timeout, socket.connect(sock_addr)).await { | ||
| Ok(res) => res, | ||
| Err(_elapsed) => { | ||
| stream = Err(GatewayClientError::NetworkConnectionTimeout { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wouldn't existing |
||
| address: endpoint.to_owned(), | ||
| timeout: connect_timeout, | ||
| }); | ||
| continue; | ||
| } | ||
| } | ||
| } else { | ||
| socket.connect(sock_addr).await | ||
| }; | ||
|
|
||
| match connect_res { | ||
| Ok(s) => { | ||
| stream = Ok(s); | ||
| break; | ||
|
|
||
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'm not sure connection timeout is a propery of a
GatewaySetup(I don't likeconnection_fd_callbackbeing there either). but if it has to be there, maybe it should be wrapped into some sort of config (alongside that callback)?