Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(client): add per IP address connection timeout #1958

Closed
wants to merge 1 commit into from
Closed

feat(client): add per IP address connection timeout #1958

wants to merge 1 commit into from

Conversation

chewi
Copy link
Contributor

@chewi chewi commented Sep 30, 2019

It has previously been recommended to apply timeouts externally to hyper. The
hyper-timeout crate uses a special connector to apply read and write timeouts
but the connection timeout is still external.

This approach has one major drawback that only an implementation within hyper
can provide. If the requested hostname resolves to multiple IP addresses, hyper
internally tries to connect to each one in turn. If one of them doesn't respond
at all then it hangs until the OS-level timeout kicks in. This defaults to 127
seconds on Linux due to tcp_syn_retries being 6. hyper-timeout wraps around all
the connection attempts rather than each one individually so setting that
timeout to something shorter means that it will give up before trying the
second address. You could reduce tcp_syn_retries but this is kernel-wide so
this is far from ideal.

Therefore add a set_connect_timeout function to HttpConnector. It defaults to
None, preserving the earlier behaviour.

Closes #1234


I'm thinking a DNS timeout would also be a good idea so I'm working on that too. hyper-timeout's connect timeout covers that part of the process. It's arguably less crucial though. The default DNS timeout with glibc is 5 seconds each for 2 tries, which isn't as bad as 127 seconds, and reducing this system-wide has less of an impact than reducing tcp_syn_retries. Please let me know if you want that work in this PR or a new one.

I need this backported to 0.12 and I'm hoping you'll accept a PR for that when it's ready.

I'm still relatively new to Rust so please go easy on me! 馃懚

@chewi
Copy link
Contributor Author

chewi commented Sep 30, 2019

I've noticed that the error you get back if all the attempts time out is quite generic.

Error: Error(Connect, Kind(TimedOut))

When working on the DNS timeout, I get the same type. Should I make it more specific or do we not really care whether it was a DNS timeout or a connect timeout?

@chewi
Copy link
Contributor Author

chewi commented Sep 30, 2019

Hmm, did Travis fail because it doesn't support IPv6? I can use IPv4 instead but 240.0.0.1 is merely reserved for future use, it's not an official black hole IP.

It has previously been recommended to apply timeouts externally to hyper. The
hyper-timeout crate uses a special connector to apply read and write timeouts
but the connection timeout is still external.

This approach has one major drawback that only an implementation within hyper
can provide. If the requested hostname resolves to multiple IP addresses, hyper
internally tries to connect to each one in turn. If one of them doesn't respond
at all then it hangs until the OS-level timeout kicks in. This defaults to 127
seconds on Linux due to tcp_syn_retries being 6. hyper-timeout wraps around all
the connection attempts rather than each one individually so setting that
timeout to something shorter means that it will give up before trying the
second address. You could reduce tcp_syn_retries but this is kernel-wide so
this is far from ideal.

Therefore add a set_connect_timeout function to HttpConnector. It defaults to
None, preserving the earlier behaviour.

Closes #1234
@chewi
Copy link
Contributor Author

chewi commented Oct 21, 2019

Closing as #1972 was merged first.

@chewi chewi closed this Oct 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add connect, read, and write timeouts to client
1 participant