Mimic GET reforwarding decisions when our CONNECT fails (#1168)

Use FwdState::reforwardableStatus() logic when deciding whether to
reforward our CONNECT request after a failed tunneling attempt.

Also honor forward_max_tries limit when retrying tunneling attempts.

These TunnelStateData fixes deal with ordinary CONNECT traffic (no
SslBump). FwdState also handles CONNECT requests (with SslBump). We make
that CONNECT handling more consistent across these classes (in addition
to making it more consistent across CONNECT and GET/etc. methods).

Co-authored-by: Alex Rousskov <rousskov@measurement-factory.com>

diff --git a/src/FwdState.cc b/src/FwdState.cc
index 8bdf5206f1d6e0534f434d13a568e6b67f045aa4..e07bea323309e595e19d2b4f8541f42d9d619f85 100644 (file)
--- a/src/FwdState.cc
+++ b/src/FwdState.cc
@@ -1339,7 +1339,7 @@ FwdState::reforward()
 
     const auto s = entry->mem().baseReply().sline.status();
     debugs(17, 3, "status " << s);
-    return reforwardableStatus(s);
+    return Http::IsReforwardableStatus(s);
 }
 
 static void
@@ -1371,32 +1371,6 @@ fwdStats(StoreEntry * s)
 
 /**** STATIC MEMBER FUNCTIONS *************************************************/
 
-bool
-FwdState::reforwardableStatus(const Http::StatusCode s) const
-{
-    switch (s) {
-
-    case Http::scBadGateway:
-
-    case Http::scGatewayTimeout:
-        return true;
-
-    case Http::scForbidden:
-
-    case Http::scInternalServerError:
-
-    case Http::scNotImplemented:
-
-    case Http::scServiceUnavailable:
-        return Config.retry.onerror;
-
-    default:
-        return false;
-    }
-
-    /* NOTREACHED */
-}
-
 void
 FwdState::initModule()
 {

diff --git a/src/FwdState.h b/src/FwdState.h
index b7adbce7c5ea01d8566842e733f869d78cb9dcdf..ad9e75cc3bc191ef052b2709a5ea63d628e544b8 100644 (file)
--- a/src/FwdState.h
+++ b/src/FwdState.h
@@ -85,7 +85,6 @@ public:
 
     void handleUnregisteredServerEnd();
     int reforward();
-    bool reforwardableStatus(const Http::StatusCode s) const;
     void serverClosed();
     void connectStart();
     void connectDone(const Comm::ConnectionPointer & conn, Comm::Flag status, int xerrno);

diff --git a/src/http.cc b/src/http.cc
index afc0b7623f12ea1ee6793794d971963c9a325103..cc6ee208e05cfa9b771019adf5486b6e7bdd774c 100644 (file)
--- a/src/http.cc
+++ b/src/http.cc
@@ -33,6 +33,7 @@
 #include "http.h"
 #include "http/one/ResponseParser.h"
 #include "http/one/TeChunkedParser.h"
+#include "http/StatusCode.h"
 #include "http/Stream.h"
 #include "HttpControlMsg.h"
 #include "HttpHdrCc.h"
@@ -967,7 +968,7 @@ HttpStateData::haveParsedReplyHeaders()
             // TODO: check whether such responses are shareable.
             // Do not share for now.
             entry->makePrivate(false);
-            if (fwd->reforwardableStatus(rep->sline.status()))
+            if (Http::IsReforwardableStatus(rep->sline.status()))
                 EBIT_SET(entry->flags, ENTRY_FWD_HDR_WAIT);
             varyFailure = true;
         } else {
@@ -986,7 +987,7 @@ HttpStateData::haveParsedReplyHeaders()
          * If its not a reply that we will re-forward, then
          * allow the client to get it.
          */
-        if (fwd->reforwardableStatus(rep->sline.status()))
+        if (Http::IsReforwardableStatus(rep->sline.status()))
             EBIT_SET(entry->flags, ENTRY_FWD_HDR_WAIT);
 
         ReuseDecision decision(entry, statusCode);

diff --git a/src/http/StatusCode.cc b/src/http/StatusCode.cc
index a4aac20ba9a8f5a00e177ba45690598311785f95..24fb2942340262a6bb287ae8a87f104eb28b0c42 100644 (file)
--- a/src/http/StatusCode.cc
+++ b/src/http/StatusCode.cc
@@ -9,6 +9,7 @@
 #include "squid.h"
 #include "debug/Stream.h"
 #include "http/StatusCode.h"
+#include "SquidConfig.h"
 
 const char *
 Http::StatusCodeString(const Http::StatusCode status)
@@ -276,3 +277,25 @@ Http::StatusCodeString(const Http::StatusCode status)
     return "Unassigned";
 }
 
+bool
+Http::IsReforwardableStatus(const StatusCode s)
+{
+    switch (s) {
+
+    case scBadGateway:
+    case scGatewayTimeout:
+        return true;
+
+    case scForbidden:
+    case scInternalServerError:
+    case scNotImplemented:
+    case scServiceUnavailable:
+        return Config.retry.onerror;
+
+    default:
+        return false;
+    }
+
+    /* NOTREACHED */
+}
+

diff --git a/src/http/StatusCode.h b/src/http/StatusCode.h
index b12a01eee63d2009a6499c4280c0521390b8cb55..38ece6d9a1b5bd90080674bfbc3f5b0096f21778 100644 (file)
--- a/src/http/StatusCode.h
+++ b/src/http/StatusCode.h
@@ -92,6 +92,8 @@ const char *StatusCodeString(const Http::StatusCode status);
 inline bool Is1xx(const int sc) { return scContinue <= sc && sc < scOkay; }
 /// whether this response status code prohibits sending Content-Length
 inline bool ProhibitsContentLength(const StatusCode sc) { return sc == scNoContent || Is1xx(sc); }
+/// whether to send the request to another peer based on the current response status code
+bool IsReforwardableStatus(StatusCode);
 
 } // namespace Http
 

diff --git a/src/tunnel.cc b/src/tunnel.cc
index 5761d11d48a89863b9b169bfc603a642804319f4..b93be36cbcda252ccd3784ab55509508be9d0e9d 100644 (file)
--- a/src/tunnel.cc
+++ b/src/tunnel.cc
@@ -31,6 +31,7 @@
 #include "globals.h"
 #include "HappyConnOpener.h"
 #include "http.h"
+#include "http/StatusCode.h"
 #include "http/Stream.h"
 #include "HttpRequest.h"
 #include "icmp/net_db.h"
@@ -188,13 +189,15 @@ public:
     time_t startTime; ///< object creation time, before any peer selection/connection attempts
     ResolvedPeersPointer destinations; ///< paths for forwarding the request
     bool destinationsFound; ///< At least one candidate path found
-    /// whether another destination may be still attempted if the TCP connection
-    /// was unexpectedly closed
-    bool retriable;
 
     /// whether the decision to tunnel to a particular destination was final
     bool committedToServer;
 
+    int n_tries; ///< the number of forwarding attempts so far
+
+    /// a reason to ban reforwarding attempts (or nil)
+    const char *banRetries;
+
     // TODO: remove after fixing deferred reads in TunnelStateData::copyRead()
     CodeContext::Pointer codeContext; ///< our creator context
 
@@ -250,6 +253,7 @@ private:
 
     bool transporting() const;
 
+    // TODO: convert to unique_ptr
     /// details of the "last tunneling attempt" failure (if it failed)
     ErrorState *savedError = nullptr;
 
@@ -260,6 +264,8 @@ private:
 
     void cancelStep(const char *reason);
 
+    bool exhaustedTries() const;
+
 public:
     bool keepGoingAfterRead(size_t len, Comm::Flag errcode, int xerrno, Connection &from, Connection &to);
     void copy(size_t len, Connection &from, Connection &to, IOCB *);
@@ -344,8 +350,9 @@ TunnelStateData::TunnelStateData(ClientHttpRequest *clientRequest) :
     startTime(squid_curtime),
     destinations(new ResolvedPeers()),
     destinationsFound(false),
-    retriable(true),
     committedToServer(false),
+    n_tries(0),
+    banRetries(nullptr),
     codeContext(CodeContext::Current())
 {
     debugs(26, 3, "TunnelStateData constructed this=" << this);
@@ -391,12 +398,20 @@ TunnelStateData::checkRetry()
 {
     if (shutting_down)
         return "shutting down";
+    if (exhaustedTries())
+        return "exhausted tries";
     if (!FwdState::EnoughTimeToReForward(startTime))
         return "forwarding timeout";
-    if (!retriable)
-        return "not retriable";
+    if (banRetries)
+        return banRetries;
     if (noConnections())
         return "no connections";
+
+    // TODO: Use Optional for peer_reply_status to avoid treating zero value specially.
+    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()
     return nullptr;
 }
 
@@ -951,6 +966,9 @@ TunnelStateData::tunnelEstablishmentDone(Http::TunnelerAnswer &answer)
 
     al->cache.code.update(LOG_TCP_TUNNEL);
 
+    // XXX: al->http.code (i.e. *status_ptr) should not be (re)set
+    // until we actually start responding to the client. Right here/now, we only
+    // know how this cache_peer has responded to us.
     if (answer.peerResponseStatus != Http::scNone)
         *status_ptr = answer.peerResponseStatus;
 
@@ -1008,7 +1026,7 @@ void
 TunnelStateData::commitToServer(const Comm::ConnectionPointer &conn)
 {
     committedToServer = true;
-    retriable = false; // may already be false
+    banRetries = "committed to server";
     PeerSelectionInitiator::subscribed = false; // may already be false
     server.initConnection(conn, tunnelServerClosed, "tunnelServerClosed", this);
 }
@@ -1034,8 +1052,12 @@ TunnelStateData::noteConnection(HappyConnOpener::Answer &answer)
 {
     transportWait.finish();
 
+    Assure(n_tries <= answer.n_tries); // n_tries cannot decrease
+    n_tries = answer.n_tries;
+
     ErrorState *error = nullptr;
     if ((error = answer.error.get())) {
+        banRetries = "HappyConnOpener gave up";
         Must(!Comm::IsConnOpen(answer.conn));
         syncHierNote(answer.conn, request->url.host());
         answer.error.clear();
@@ -1062,6 +1084,8 @@ TunnelStateData::connectDone(const Comm::ConnectionPointer &conn, const char *or
         ResetMarkingsToServer(request.getRaw(), *conn);
     // else Comm::ConnOpener already applied proper/current markings
 
+    // TODO: add pconn race state tracking
+
     syncHierNote(conn, origin);
 
 #if USE_DELAY_POOLS
@@ -1090,6 +1114,13 @@ TunnelStateData::connectDone(const Comm::ConnectionPointer &conn, const char *or
     }
 }
 
+/// whether we have used up all permitted forwarding attempts
+bool
+TunnelStateData::exhaustedTries() const
+{
+    return n_tries >= Config.forward_max_tries;
+}
+
 void
 tunnelStart(ClientHttpRequest * http)
 {
@@ -1357,8 +1388,13 @@ TunnelStateData::startConnecting()
 
     assert(!destinations->empty());
     assert(!transporting());
+
+    delete savedError; // may still be nil
+    savedError = nullptr;
+    request->hier.peer_reply_status = Http::scNone; // TODO: Move to startPeerClock()?
+
     const auto callback = asyncCallback(17, 5, TunnelStateData::noteConnection, this);
-    const auto cs = new HappyConnOpener(destinations, callback, request, startTime, 0, al);
+    const auto cs = new HappyConnOpener(destinations, callback, request, startTime, n_tries, al);
     cs->setHost(request->url.host());
     cs->setRetriable(false);
     cs->allowPersistent(false);
@@ -1376,6 +1412,8 @@ TunnelStateData::usePinned()
         const auto serverConn = ConnStateData::BorrowPinnedConnection(request.getRaw(), al);
         debugs(26, 7, "pinned peer connection: " << serverConn);
 
+        ++n_tries;
+
         // Set HttpRequest pinned related flags for consistency even if
         // they are not really used by tunnel.cc code.
         request->flags.pinned = true;