Remove doomed attempts to reforward pinned requests (#2449)

    AsyncJob.cc(100) mustStop: Ftp::Relay will stop, reason: exception
    ...
    FATAL: check failed: !request->pinnedConnection()
        exception location: FwdState.cc(1119) connectStart

Since February 2019 commit 3dde9e52, retries of "pinned" requests[^1]
using a new Squid-to-server connections are meant for FTP and
connection-based authentication traffic only. Back then, we agreed that
such re-pinning is wrong[^2], and different mechanisms should be used to
handle forwarding errors in those cases, but we preserved the
corresponding functionality in fear of breaking "working" cases[^3]. On
that preserved code path, `request->pinnedConnection()` is true at
pinnedCanRetry() check time.

Since September 2019 commit daf80700, connectStart() throws if
`request->pinnedConnection()` is true, effectively breaking retries for
nearly all preserved cases mentioned above! The only pinned retries that
could hypothetically continue working after that commit are those where
ConnStateData job had been destroyed between pinnedCanRetry() and
connectStart(), and those rare cases are not interesting since
ConnStateData destruction ought to abort request forwarding anyway.

Both 2019 commits are present in v5.0.1 and beyond. We are not aware of
any associated problems. Thus, our fears were excessive, and we can
remove code that preserves problematic functionality. If we discover a
need to retry pinned FTP or connection-authenticated requests via a new
connection, we will add mechanisms to support that functionality.

Removing this code also helps avoid FATAL errors observed during recent
FTP work.

CONNECT tunnels
---------------

Existing tunnel.cc code ignores `request->flags.pinned` when deciding
whether to retry a failed tunneling attempt. This change does not affect
the corresponding "check pinned connections" TODO in that tunneling
code. FWIW, that TODO is limited to tunnelStart(); switchToTunnel()
cases already do not retry due to a "committed to server" retry ban on
their code path. It is likely that pinned retries should be disabled in
tunneled cases as well, but that adjustment deserves a dedicated
analysis/change.

[^1]: Here, a "pinned" request is, roughly speaking, a request sent by
Squid over a Squid-to-server connection that was kept/provided by a
client-to-Squid connection manager (ConnStateData). See usePinned().

[^2]: "automatically re-opening a PINNED connection from the middleware
is invalid in todays networks" at
https://github.com/squid-cache/squid/pull/351#discussion_r250015937

[^3]: "keep re-pinning as is" at
https://github.com/squid-cache/squid/pull/351#discussion_r253757986

diff --git a/src/FwdState.cc b/src/FwdState.cc
index 62c33a67f845415a091dafa8f8f4d74ebb598bf7..479fd1082a50dc9911ac2ecc15283a18b304b214 100644 (file)
--- a/src/FwdState.cc
+++ b/src/FwdState.cc
@@ -708,7 +708,7 @@ FwdState::checkRetry()
     if (exhaustedTries())
         return false;
 
-    if (request->flags.pinned && !pinnedCanRetry())
+    if (request->flags.pinned)
         return false;
 
     if (!EnoughTimeToReForward(start_t))
@@ -1323,7 +1323,7 @@ FwdState::reforward()
 
     debugs(17, 3, e->url() << "?" );
 
-    if (request->flags.pinned && !pinnedCanRetry()) {
+    if (request->flags.pinned) {
         debugs(17, 3, "pinned connection; cannot retry");
         return 0;
     }
@@ -1418,28 +1418,6 @@ FwdState::exhaustedTries() const
     return n_tries >= Config.forward_max_tries;
 }
 
-bool
-FwdState::pinnedCanRetry() const
-{
-    assert(request->flags.pinned);
-
-    // pconn race on pinned connection: Currently we do not have any mechanism
-    // to retry current pinned connection path.
-    if (pconnRace == raceHappened)
-        return false;
-
-    // If a bumped connection was pinned, then the TLS client was given our peer
-    // details. Do not retry because we do not ensure that those details stay
-    // constant. Step1-bumped connections do not get our TLS peer details, are
-    // never pinned, and, hence, never reach this method.
-    if (request->flags.sslBumped)
-        return false;
-
-    // The other pinned cases are FTP proxying and connection-based HTTP
-    // authentication. TODO: Do these cases have restrictions?
-    return true;
-}
-
 time_t
 FwdState::connectingTimeout(const Comm::ConnectionPointer &conn) const
 {

diff --git a/src/FwdState.h b/src/FwdState.h
index 1427b928ebba428ead669df48eaf4c154b92c148..1dc2d4e3b70471ae3485c0e94e1d4cda37531ac6 100644 (file)
--- a/src/FwdState.h
+++ b/src/FwdState.h
@@ -161,10 +161,6 @@ private:
 
     void usePinned();
 
-    /// whether a pinned to-peer connection can be replaced with another one
-    /// (in order to retry or reforward a failed request)
-    bool pinnedCanRetry() const;
-
     template <typename StepStart>
     void advanceDestination(const char *stepDescription, const Comm::ConnectionPointer &conn, const StepStart &startStep);
 

diff --git a/src/tunnel.cc b/src/tunnel.cc
index 5da201b6bd672b8a201ff21d0b8c9907d521a746..041ae0be5957c2c18b0e0e96f2aed3fab6ee88c3 100644 (file)
--- a/src/tunnel.cc
+++ b/src/tunnel.cc
@@ -517,7 +517,7 @@ TunnelStateData::checkRetry()
     if (request->hier.peer_reply_status != Http::scNone && !Http::IsReforwardableStatus(request->hier.peer_reply_status))
         return "received HTTP status code is not reforwardable";
 
-    // TODO: check pinned connections; see FwdState::pinnedCanRetry()
+    // TODO: check pinned connections; see request->flags.pinned in FwdState::checkRetry()
     return nullptr;
 }