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

Race TCP and UDP upstream connections instead of trying sequentially #1266

Closed
ThinkChaos opened this issue Nov 21, 2023 · 2 comments · Fixed by #1302
Closed

Race TCP and UDP upstream connections instead of trying sequentially #1266

ThinkChaos opened this issue Nov 21, 2023 · 2 comments · Fixed by #1302
Assignees
Labels
🔨 enhancement New feature or request
Milestone

Comments

@ThinkChaos
Copy link
Collaborator

Right now when a client uses TCP, we try TCP and then UDP if that failed to connect. I think we should race both connections and use the winner (obviously if the UDP response wins but is truncated we shouldn't use it). That's similar in concept to the Happy Eyeballs algorithm.
This would also have the benefit of falling back to UDP for any kind of failure, not just connection refused. I think this could make a difference in practice since a lot of firewalls are set to DROP by default.

At the same time, we should investigate and fix the inconsistency between dnsUpstreamClient.callExternal and createUpstreamClient: the later always creates the struct with both tcpClient and udpClient, but callExternal seems to have been written before that was the case as it does nil checks.

See #1261 (comment)

@ThinkChaos ThinkChaos added the 🔨 enhancement New feature or request label Nov 21, 2023
@ThinkChaos ThinkChaos self-assigned this Nov 21, 2023
@ThinkChaos
Copy link
Collaborator Author

In #1261, I said:

When writing the test, I realized this isn't an issue: the only case we retry is in case of an error during "dial", aka connection refused which shouldn't take long so we'll be fine.

Well I was wrong, dial actually includes more cases than that, so I think this should be addressed or reverted to not use a context before the next release.
I'll take a look today at racing the connections, to estimate if I can do it quick enough. Either way I'll make a PR in the next couple of days so we can make a release without this being broken.

@0xERR0R 0xERR0R added this to the v0.23 milestone Dec 13, 2023
@ThinkChaos
Copy link
Collaborator Author

At the same time, we should investigate and fix the inconsistency between dnsUpstreamClient.callExternal and createUpstreamClient: the later always creates the struct with both tcpClient and udpClient, but callExternal seems to have been written before that was the case as it does nil checks.

When I wrote this I missed the TLS case where UDP is unset. So I'm keeping that part of the logic intact.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants