[squid-dev] [PATCH] Happy Eyeballs: Use each fully resolved destination ASAP

[Amos Jeffries]

On 16/05/17 16:33, Alex Rousskov wrote:
> Hello,
>
>      The attached patch is the first in a short series of patches that
> improve Squid support for the Happy Eyeballs principle. The motivation
> for these patches has been discussed on the dns_wait_for_all thread:
> http://lists.squid-cache.org/pipermail/squid-dev/2016-September/006831.html

NP: This is not implementing the Happy Eyeballs *algorithm* (it is an 
explicitly standardized algorithm name, not a principle). While it 
enables parallel lookup and use of different paths, the individual path 
IPs remain ordered such that dns_v4_first is still relevant. "Happy 
Eyeballs" algorithm is specifically a DNS feature.

> With this change, Squid no longer waits for all request forwarding
> destinations to be resolved. Instead, it uses each fully resolved
> destination as soon as it is needed. More info is in the patch preamble.

More correctly this code does not even begin to resolve some 
destinations until others are already started being connected to. It is 
a step back to non-deterministic forwarding timeouts.

> The next planned step is to deliver A and AAAA answers to the peer
> selection code independently and ASAP. When both changes are combined,
> the FwdState/tunneling code will receive IPvX addresses without waiting
> for IPvY addresses (and/or other destination names) to be resolved,
> making user eyeballs happier and, hence, facilitating IPv6 deployments.

*That* is the missing part of Happy Eyeballs. But as you note, is not 
included in this patch.

> This patch also adds reporting of peer type when dumping Connection
> info. I removed operator "<<" definition from Connection.h header
> because we should not burden all Connection users with these low-level
> (and performance-unrelated) details. I also removed associated comments
> because they were partially misleading (or perhaps outdated) and
> partially misplaced (Connection.h is not the right place to state basic
> C++ operator overloading rules). If there are no objections, I will
> commit this minor improvement separately.
>
> No changes to FwdServer class but its declaration has been moved from
> the PeerSelectState.h header to peer_select.cc because FwdServer is
> unused by peerSelect() users.
>
> No changes to peerCountMcastPeers functionality. I only documented the
> abuse and marked the abused ps_state field with XXX to prevent future
> abuses.
>

Audit:


in src/FwdState.cc:

* s/Will aborting forwarding/Will abort forwarding/


in src/peer_select.cc:

* psstate->lastError = NULL; // initiator owns the ErrorState object now
  - please use nullptr on moved or new lines

* The call to noteDestinationsEnd() with an error from peerSelectDnsResults() appears to abort *all* path connection attempts by the initiator with an error page if

  a) the first path had an error, or

  b) the DNS lookup for the path was slow enough that all previous paths
were tried and emptied from serverDestinations.

eg. for an IPv4-only network when the first N peers are unreachable (very fast TCP failover) and the Nth does not have
ipcache listed records (slow remote lookup).


Amos