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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(client): Add connect timeout to HttpConnector #1972

Merged
merged 1 commit into from
Oct 14, 2019

Conversation

sfackler
Copy link
Contributor

This takes the same strategy as golang, where the timeout value is
divided equally between the candidate socket addresses.

If happy eyeballs is enabled, the division takes place "below" the
IPv4/IPv6 partitioning.

This takes the same strategy as golang, where the timeout value is
divided equally between the candidate socket addresses.

If happy eyeballs is enabled, the division takes place "below" the
IPv4/IPv6 partitioning.
Copy link
Member

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me! Is there a small-ish test we can come up with? I suppose simulating slow TCP connects is hard...

(Also, wow this file could use some cleaning by converting to async fns XD)

@sfackler
Copy link
Contributor Author

Yeah - we ended up removing connect timeout tests from the standard library because they just didn't work well. It turns out that there are apparently tons of networks that even route IPs that are designated unroutable :(

@seanmonstar
Copy link
Member

That might explain why the happy eyeballs tests seem to work in some environments and not others.

@seanmonstar seanmonstar merged commit 4179297 into hyperium:master Oct 14, 2019
@sfackler sfackler deleted the connect-timeout branch October 14, 2019 18:49
@chewi
Copy link
Contributor

chewi commented Oct 16, 2019

Somehow this arrived after my #1958 but got merged first. 😢 We've done almost exactly the same thing.

I didn't divide the timeout by the number of addresses. I'm not sure if that makes sense as you may not be in control of how many addresses get returned and a short timeout could mean that every attempt will be shorter than any one server can handle.

The error handling was a bit simpler in my version but I'm not sure if it actually has the same result.

I did add a unit test but given what you've said above, I guess that should be left out? It initially used 100::1, which is officially unrouteable, but Travis CI doesn't have IPv6 so I changed it to 240.0.0.1.

I was going to follow it up with a resolve timeout. I believe there is still a need for that.

I also backported both the connect timeout and the resolve timeout to 0.12. Would that be accepted?

Please advise on how I should proceed.

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.

3 participants