Add Happy Eyeballs support#176
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds Happy Eyeballs (RFC 8305) support to resolve issue #172, which occurred when IPv6-only connection attempts failed on IPv4-only machines. The implementation restores dual-stack connectivity that was unintentionally lost in PR #157 when DNS resolution was optimized. The changes enable the client to resolve both IPv4 and IPv6 addresses and race them with IPv6 given a 300ms head start, using the first successful connection.
Changes:
- Modified DNS resolution to return both IPv4 and IPv6 addresses instead of a single IP
- Implemented
dialParallelfunction that races IPv4 and IPv6 connections per RFC 8305 - Updated DNS caching to store both address families in a
dnsResultstruct - Added comprehensive tests for IPv4/IPv6 disabling scenarios and cache interactions
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| dns.go | Modified archiveDNS to return both IPv4 and IPv6 addresses; added dnsResult struct for caching both address families |
| dialer.go | Implemented Happy Eyeballs via dialParallel and dialSingle functions; removed getNetworkType; updated dial contexts to use parallel racing |
| dns_test.go | Updated all test calls for new archiveDNS signature; added extensive tests for IPv4/IPv6 disable flags and cache behavior |
| dialer_test.go | Removed TestGetNetworkType as the function no longer exists |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
willmhowes
left a comment
There was a problem hiding this comment.
I admittedly did not study the test cases very closely, but the dialer and dns changes look good!
| // It returns the first established connection and closes the other. | ||
| // Otherwise it returns an error from the primary address. | ||
| // This implements Happy Eyeballs (RFC 8305). | ||
| func (d *customDialer) dialParallel(ctx context.Context, network string, primaryAddr, fallbackAddr string, primaryIP, fallbackIP net.IP) (net.Conn, net.IP, error) { |
There was a problem hiding this comment.
Took me a minute to figure out what was going on with the "network" string throughout the code, but I can see now that's it's usually meant to represent the TCP or UDP protocol, with a suffix of 4 or 6 appended before ultimately being sent to d.DialContext
| } | ||
| } | ||
|
|
||
| return d.DialContext(ctx, network, address) |
There was a problem hiding this comment.
This is more of a problem I have with the golang std lib net package, but for clarify of code I believe it would be nice if network was defined as a custom type or enum, rather than an untyped string. Non an actionable request, just a thought
Resolves #172
This issue was a result of #157 dialing to the IP address directly, rather than the DNS name (which previously resulted in a secondary lookup anyways...). In fixing it, we removed our (unintentional) Happy Eyeballs support. This adds it back explicitly.