From 3eebd267df415536641878d692117caca30ca069 Mon Sep 17 00:00:00 2001 From: Eduard Bagdasaryan Date: Wed, 28 Nov 2018 23:55:29 +0000 Subject: [PATCH] Fixed forward_max_tries documentation and implementation (#277) Before 1c8f25b, FwdState::n_tries counted the total number of forwarding attempts, including pinned and persistent connection retries. Since that revision, it started counting just those retries. What should n_tries count? The counter is used to honor the forward_max_tries directive, but that directive was documented to limit the number of _different_ paths to try. Neither 1c8f25b~1 nor 1c8f25b code matched that documentation! Continuing to count just pinned and persistent connection retries (as in 1c8f25b) would violate any reasonable forward_max_tries intent and admin expectations. There are two ways to fix this problem, synchronizing code and documentation: * Count just the attempts to use a different forwarding path, matching forward_max_tries documentation but not what Squid has ever done. This approach makes it difficult for an admin to limit the total number of forwarding attempts in environments where, say, the second attempt is unlikely to succeed and will just incur wasteful delays (Squid bug 4788 report is probably about one of such use cases). Also, implementing this approach may be more difficult because it requires adding a new counter for retries and, for some interpretations of "different", even a container of previously visited paths. * Count all forwarding attempts (as before 1c8f25b) and adjust forward_max_tries documentation to match this historical behavior. This approach does not have known unique flaws. Also fixed FwdState::n_tries off-by-one comparison bug discussed during Squid bug 4788 triage. Also fixed admin concern behind Squid bug 4788 "forward_max_tries 1 does not prevent some retries": While the old forward_max_tries documentation actually excluded pconn retries, technically invalidating the bug report, the admin now has a knob to limit those retries. --- src/FwdState.cc | 11 +++++++++-- src/FwdState.h | 5 ++++- src/cf.data.pre | 36 ++++++++++++++++++++++-------------- 3 files changed, 35 insertions(+), 17 deletions(-) diff --git a/src/FwdState.cc b/src/FwdState.cc index 5a785e3663..2edada93ca 100644 --- a/src/FwdState.cc +++ b/src/FwdState.cc @@ -626,7 +626,7 @@ FwdState::checkRetry() if (!entry->isEmpty()) return false; - if (n_tries > Config.forward_max_tries) + if (exhaustedTries()) return false; if (!EnoughTimeToReForward(start_t)) @@ -960,6 +960,7 @@ FwdState::connectStart() Comm::ConnOpener *cs = new Comm::ConnOpener(serverDestinations[0], calls.connector, connTimeout); if (host) cs->setHost(host); + ++n_tries; AsyncJob::Start(cs); } @@ -1111,7 +1112,7 @@ FwdState::reforward() return 0; } - if (n_tries > Config.forward_max_tries) + if (exhaustedTries()) return 0; if (request->bodyNibbled()) @@ -1261,6 +1262,12 @@ FwdState::logReplyStatus(int tries, const Http::StatusCode status) ++ FwdReplyCodes[tries][status]; } +bool +FwdState::exhaustedTries() const +{ + return n_tries >= Config.forward_max_tries; +} + /**** PRIVATE NON-MEMBER FUNCTIONS ********************************************/ /* diff --git a/src/FwdState.h b/src/FwdState.h index 2baa0df969..403761cdbc 100644 --- a/src/FwdState.h +++ b/src/FwdState.h @@ -133,6 +133,9 @@ private: void syncWithServerConn(const char *host); void syncHierNote(const Comm::ConnectionPointer &server, const char *host); + /// whether we have used up all permitted forwarding attempts + bool exhaustedTries() const; + public: StoreEntry *entry; HttpRequest *request; @@ -145,7 +148,7 @@ private: ErrorState *err; Comm::ConnectionPointer clientConn; ///< a possibly open connection to the client. time_t start_t; - int n_tries; + int n_tries; ///< the number of forwarding attempts so far // AsyncCalls which we set and may need cancelling. struct { diff --git a/src/cf.data.pre b/src/cf.data.pre index 070b7f068b..3df11e2095 100644 --- a/src/cf.data.pre +++ b/src/cf.data.pre @@ -3932,11 +3932,15 @@ DEFAULT: 25 TYPE: int LOC: Config.forward_max_tries DOC_START - Controls how many different forward paths Squid will try - before giving up. See also forward_timeout. + Limits the number of attempts to forward the request. + + 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. - NOTE: connect_retries (default: none) can make each of these - possible forwarding paths be tried multiple times. + See also: forward_timeout and connect_retries. DOC_END COMMENT_START @@ -10095,19 +10099,23 @@ LOC: Config.connect_retries DEFAULT: 0 DEFAULT_DOC: Do not retry failed connections. DOC_START - This sets the maximum number of connection attempts made for each - TCP connection. The connect_retries attempts must all still - complete within the connection timeout period. + Limits the number of reopening attempts when establishing a single + TCP connection. All these attempts must still complete before the + applicable connection opening timeout expires. + + By default and when connect_retries is set to zero, Squid does not + retry failed connection opening attempts. - The default is not to re-try if the first connection attempt fails. - The (not recommended) maximum is 10 tries. + The (not recommended) maximum is 10 tries. An attempt to configure a + higher value results in the value of 10 being used (with a warning). - A warning message will be generated if it is set to a too-high - value and the configured value will be over-ridden. + Squid may open connections to retry various high-level forwarding + failures. For an outside observer, that activity may look like a + low-level connection reopening attempt, but those high-level retries + are governed by forward_max_tries instead. - Note: These re-tries are in addition to forward_max_tries - which limit how many different addresses may be tried to find - a useful server. + See also: connect_timeout, forward_timeout, icap_connect_timeout, + ident_timeout, and forward_max_tries. DOC_END NAME: retry_on_error -- 2.39.5