Skip to content

feat(client): implement rfc 6555 (happy eyeballs) #1598

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

Merged
merged 1 commit into from
Jul 10, 2018
Merged

feat(client): implement rfc 6555 (happy eyeballs) #1598

merged 1 commit into from
Jul 10, 2018

Conversation

hban
Copy link
Contributor

@hban hban commented Jul 8, 2018

First PR, criticism welcome!

Update client connector to attempt a parallel connection using alternative address family, if connection using preferred address family takes too long.

Some questions:

  • What should be a default value for timeout before we attempt other connections? Currently is 300ms.
  • How to write a test for this? For manual testing I used edited /etc/hosts and iptables to simulate non-functioning IPv6. Maybe same could be done for Travis? Unsure about Appveyor.

Closes: #1316

@seanmonstar
Copy link
Member

Awesome work, thanks! With regards to testing, I peeked at what Golang does. They make use of some special reserved IP addresses to test their happy eyeballs implementation: https://github.com/golang/go/blob/master/src/net/dial_test.go#L136

Would that help inspire some tests here?

@scrogson
Copy link
Contributor

scrogson commented Jul 8, 2018

It appears that RFC 6555 has been obsoleted by https://tools.ietf.org/html/rfc8305

@hban
Copy link
Contributor Author

hban commented Jul 10, 2018

I've added a test, but I'm not really happy how it turned out. I needed to provide custom name resolver, which Hyper doesn't support, so I hacked one in, hidden behind non-default feature flag. It's crude and ugly solution.

Instead, I can write a test directly for ConnectingTcp. It would remove need for this test-only custom resolver, but would mix this integration test with other unit tests.

Anyway, I'm not really sure which options is better, or if there is a third one I missed.

Regarding RFC 8305 @scrogson mentioned, I see it as a extension of work done here (correct me if I'm wrong). DNS part would require support for querying A records separately from AAAA records. ToSocketAddrs doesn't support specifying address family AFAIK, so we would need some other resolver. This would also impact implementing custom resolvers since they would either need to receive address family as parameter or return pair-of-futures/stream. Staggering connections could be done now, but I'd prefer for them to be a separate PR (if they are desired feature).

@hban
Copy link
Contributor Author

hban commented Jul 10, 2018

Apparently Travis doesn't enable IPv6 by default (travis-ci/travis-ci#8361), so I copied workaround from one of the comments to make it work. I hope that's OK.

@seanmonstar
Copy link
Member

Instead, I can write a test directly for ConnectingTcp. It would remove need for this test-only custom resolver, but would mix this integration test with other unit tests.

I think this way sounds much better! I have no problem with inner tests when testing very specific behavior that is difficult to simulate in the integration tests.

Update client connector to attempt a parallel connection using
alternative address family, if connection using preferred address family
takes too long.

Closes: #1316
@hban
Copy link
Contributor Author

hban commented Jul 10, 2018

Done (and commits squashed).

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.

Phenomenal! The code is clear, the comments are useful for "why", and test is thorough! ❤️

@seanmonstar seanmonstar merged commit 02a9c29 into hyperium:master Jul 10, 2018
@mehcode
Copy link

mehcode commented Jul 10, 2018

@seanmonstar @hban RFC 8305 (which obsoletes RFC 6555) seems to recommend 250ms for the timeout (section 5).

I'm not super clear on what big changes are between 6555 and 8305 but it might be worth looking into.

https://tools.ietf.org/html/rfc8305


See golang/go#23841 for some more concrete details


Really awesome work by the way 💯 I can remove a bunch of hacks I currently have.

@hban hban deleted the gh1316-happy-eyeballs branch July 11, 2018 16:19
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.

4 participants