Add connect timeout #6110
Add connect timeout #6110
Conversation
pronebird
force-pushed
the
am/ws-set-conn-timeout
branch
from
1ee18e9 to
889465d
Compare
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
pronebird
force-pushed
the
am/ws-set-conn-timeout
branch
from
889465d to
7f7b701
Compare
pronebird
force-pushed
the
am/ws-set-conn-timeout
branch
from
7f7b701 to
6156e26
Compare
There was a problem hiding this comment.
And as for wasm, that's a tricky one because there's no underlying connect_async function as internally it just calls browser's WebSocket and relies on callbacks for knowing when the connection has been opened. And gloo's implementation (the library we're using) is using a blocking call. Perhaps you could introduce js' timeout and cancel the socket in that case?
| available_gateways, | ||
| #[cfg(unix)] | ||
| connection_fd_callback: None, | ||
| connect_timeout: None, |
There was a problem hiding this comment.
I'm not sure connection timeout is a propery of a GatewaySetup (I don't like connection_fd_callback being there either). but if it has to be there, maybe it should be wrapped into some sort of config (alongside that callback)?
| self | ||
| } | ||
|
|
||
| pub fn with_connect_timeout(mut self, timeout: Duration) -> Self { |
There was a problem hiding this comment.
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. gateway_connection section inside the DebugConfig sounds like the perfect place for it, wouldn't you say?
| 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.
and then I guess (with my previous comment in mind), all of those extra changes could be avoided
| use std::net::SocketAddr; | ||
|
|
||
| #[cfg(not(target_arch = "wasm32"))] | ||
| pub(crate) async fn connect_async( |
There was a problem hiding this comment.
while going through the code, I've noticed we have identical connect_async function in common/client-core/src/init/websockets.rs (don't ask me why - I wish I knew). and actually when you go to where it is being called (the duplicate), it's already wrapped in a timeout - perhaps we could remove some duplication there?
| 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.
wouldn't existing GatewayConnectionTimeout variant be more appropriate?
This is a draft PR that adds connect timeout for websocket. Since
tokio::net::TcpSocketdoes not supportconnect_timeout()from std, the next close alternative usingtokio::time::timeout()is used instead.Connect timeout only limits how much time the call to
TcpSocket::connect()may take before giving up.A few notes:
This change is