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

Apply the Happy Eyeballs philosophy to parallel c-ares queries #3699

Closed
wants to merge 4 commits into from

Conversation

@b-spencer
Copy link
Contributor

commented Mar 22, 2019

When it supports both IPv4 and IPv6, cURL follows the happy eyeballs algorithm by trying to connect via both protocols in a staggered fashion. When cURL is configured to use c-ares, this also means doing two simultaneous, parallel DNS requests, one for the A record and one for the AAAA record, so that cURL can have both the IPv4 and IPv6 address in hand. (Ideally, there'd be a single request, but that's not the case for a variety of reasons.)

Today, cURL issues both of these requests at the same time, which is good. However, this parallelism is encapsulated within the c-ares driver, so the larger cURL logic is unaware that there are actually two requests and not just a single (combined A and AAAA) DNS request. The rest of cURL waits for this meta request to finish before it tries to start any connections.

Unfortunately, when either one of the parallel c-ares requests takes a long time to finish, cURL sits and waits for it, even if the c-ares driver does have a viable DNS result from the other request. Practically speaking, this means that if you are on a network that claims to support IPv6 and allows binding such sockets and addresses locally, but whose DNS server is misconfigured and can't properly respond to AAAA requests, you can find yourself in a situation where you have the usable A record in hand, but you're waiting for a series of timeouts and/or failures from all the malfunctioning DNS servers in the list.

This PR makes a small tweak to the existing c-ares driver that takes same "happy eyeballs" philosophy from the actual TCP connections (connect to one and then after a short timeout, try the other) and it applies it to the parallel DNS requests themselves. With this change, once a usable DNS response is in hand (be it A or AAAA), c-ares will only wait a short time before giving up on the second response and just going with what it has.

The exact algorithm and rationale are documented in the comments inside the PR. The changes were specifically chosen to be simple and not overly aggressive. This might not be the best "fix", but it seems better than the status quo, and I hope that it can at least be the start of a discussion on how to handle real-world cases like this.

b-spencer added 4 commits Jan 2, 2019
Merge from upstream curl
Pull upstream master
Pull upstream master
@bagder
bagder approved these changes Mar 22, 2019
Copy link
Member

left a comment

Sounds like an excellent idea and approach!

@bagder

This comment has been minimized.

Copy link
Member

commented Mar 24, 2019

the only test fail is the libssh2 test failure which is unrelated (but I don't know why it started fail in all CI builds...)

@bagder

This comment has been minimized.

Copy link
Member

commented Mar 24, 2019

Just to play this safe as there's a release coming in just a few days, I want to merge this after the pending release to give this slightly more time to cook before it ships.

/* How long we are willing to wait for additional parallel responses after
obtaining a "definitive" one.
This is intended to equal the c-ares default timeout. cURL always uses that

This comment has been minimized.

Copy link
@danielgustafsson

danielgustafsson Mar 24, 2019

Member

This should be "curl", but that nitpick can be fixed by the committer rather than require a fresh rebase IMHO.

This comment has been minimized.

Copy link
@b-spencer

b-spencer Mar 25, 2019

Author Contributor

Oops. I've been spelling it what I guess is the old-fashioned way!

Thanks!

BTW, maybe someone should fix the Wikipedia article's spelling :)

@bagder

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

Thanks!

@bagder bagder closed this in 80208d6 Mar 27, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants
You can’t perform that action at this time.