Discarded connections do not contribute to forward_max_tries (#1093)

The forward_max_tries directive is said to limit the number of
"high-level request forwarding attempts", but its documentation does not
specify whether each Happy Eyeballs connection opening attempt
constitutes such a forwarding attempt (because commit 3eebd26 that
clarified forward_max_tries meaning came before Happy Eyeballs code
started opening multiple concurrent connections in commit 5562295).

Official Squid counts each Happy Eyeballs connection attempt as a
request forwarding attempt. For example, if forward_max_tries is 2, then
Squid, to a likely admin surprise, may refuse to re-forward the request
after an HTTP failure because the second request forwarding attempt has
been used up by the (canceled!) spare TCP connection opening attempt.

This change stops counting discarded connections as request forwarding
attempts. For example, if forward_max_tries is 2, Squid will re-forward
the request (if needed and possible) even if Squid must open up to 3
connections to do that (discarding the second one in the process). In
this context, discarding the race-losing connection and going with the
winning one may be viewed as a low-level retry activity that
forward_max_tries is already documented to ignore.

Eventually, Squid may stop discarding connections that lost the Happy
Eyeballs race. When/if that complicated improvement is implemented,
those canceled connection opening attempts should be counted, but the
then-saved connections should be used for request re-forwarding,
preserving the same overall outcome: With forward_max_tries set to 2,
Squid will still re-forward the request (if needed and possible).

Before this change, setting forward_max_tries to 1 prevented all spare
connection attempts: The first DNS response received (A or AAAA) would
be used for the primary connection attempt, increasing n_tries, making
ranOutOfTimeOrAttempts() true, and blocking any spare attempt (both
concurrent and sequential). That low-level side effect is as wrong as
the blocked retries described in the "likely admin surprise" example
above. Now, admins that really want to disable spare connection attempts
may set forward_max_tries to 1 _and_ happy_eyeballs_connect_limit to 0.

diff --git a/src/HappyConnOpener.cc b/src/HappyConnOpener.cc
index 6d83ff14f5d254b7a7e4aec920638909075b1156..a9f2df589679caba65011f18e8cd128c172d3355 100644 (file)
--- a/src/HappyConnOpener.cc
+++ b/src/HappyConnOpener.cc
@@ -568,8 +568,6 @@ HappyConnOpener::openFreshConnection(Attempt &attempt, PeerConnectionPointer &de
     const auto conn = dest->cloneProfile();
     GetMarkingsToServer(cause.getRaw(), *conn);
 
-    ++n_tries;
-
     typedef CommCbMemFunT<HappyConnOpener, CommConnectCbParams> Dialer;
     AsyncCall::Pointer callConnect = asyncCall(48, 5, attempt.callbackMethodName,
                                      Dialer(this, attempt.callbackMethod));
@@ -611,6 +609,8 @@ HappyConnOpener::handleConnOpenerAnswer(Attempt &attempt, const CommConnectCbPar
     handledPath.finalize(params.conn); // closed on errors
     attempt.finish();
 
+    ++n_tries;
+
     if (params.flag == Comm::OK) {
         sendSuccess(handledPath, false, what);
         return;

diff --git a/src/HappyConnOpener.h b/src/HappyConnOpener.h
index c57c431ad8a43360ecf2bb5008036373c4157630..63e4df9e7730e2f9a23abeaae25a1a94afbea67c 100644 (file)
--- a/src/HappyConnOpener.h
+++ b/src/HappyConnOpener.h
@@ -258,7 +258,8 @@ private:
     /// the request that needs a to-server connection
     HttpRequestPointer cause;
 
-    /// number of connection opening attempts, including those in the requestor
+    /// number of our finished connection opening attempts (including pconn
+    /// reuses) plus previously finished attempts supplied by the requestor
     int n_tries;
 
     /// Reason to ran out of time or attempts

diff --git a/src/cf.data.pre b/src/cf.data.pre
index 48f3e1319d79acb8292f36093303f105c6e9eac4..a0bdb2f83f3f58b22a4b39adc9bb2b5e6c98e866 100644 (file)
--- a/src/cf.data.pre
+++ b/src/cf.data.pre
@@ -4004,8 +4004,10 @@ DOC_START
        For the purpose of this limit, Squid counts all high-level request
        forwarding attempts, including any same-destination retries after
        certain persistent connection failures and any attempts to use a
-       different peer. However, low-level connection reopening attempts
-       (enabled using connect_retries) are not counted.
+       different peer. However, these low-level attempts are not counted:
+       * connection reopening attempts (enabled using connect_retries)
+       * unfinished Happy Eyeballs connection attempts (prevented by setting
+         happy_eyeballs_connect_limit to 0)
 
        See also: forward_timeout and connect_retries.
 DOC_END