From a137484f5883cf57b9793cd3df0e84e10bcb4e2e Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Tue, 19 Jul 2022 21:25:32 +0000 Subject: [PATCH] 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. --- src/HappyConnOpener.cc | 4 ++-- src/HappyConnOpener.h | 3 ++- src/cf.data.pre | 6 ++++-- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/HappyConnOpener.cc b/src/HappyConnOpener.cc index 6d83ff14f5..a9f2df5896 100644 --- 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 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 c57c431ad8..63e4df9e77 100644 --- 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 48f3e1319d..a0bdb2f83f 100644 --- 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 -- 2.47.2