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.

diff --git a/src/FwdState.cc b/src/FwdState.cc
index 5a785e3663e2637dff2bdb54e27207c2189809f2..2edada93ca409ac58e6850a43a8811e7ac6997dd 100644 (file)
--- 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 2baa0df9693df1bd11db69d8cffd7a47a4985a03..403761cdbc53763001910e98aee511a7d200b253 100644 (file)
--- 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 070b7f068b8107df707133f5b3ace63a48713580..3df11e2095b2ffc41659d45545386e5c0a9dc7f2 100644 (file)
--- 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