diff --git a/src/CachePeer.cc b/src/CachePeer.cc index e6f4b0d..2a43cb4 100644 --- a/src/CachePeer.cc +++ b/src/CachePeer.cc @@ -10,6 +10,7 @@ #include "acl/Gadgets.h" #include "CachePeer.h" #include "defines.h" +#include "neighbors.h" #include "NeighborTypeDomainList.h" #include "pconn.h" #include "PeerPoolMgr.h" @@ -46,3 +47,43 @@ CachePeer::~CachePeer() xfree(domain); } +void +CachePeer::noteSuccess() +{ + if (!tcp_up) { + debugs(15, 2, "connection to " << this->host << "/" << this->http_port << " succeeded"); + tcp_up = connect_fail_limit; // NP: so peerAlive() works properly. + peerAlive(this); + } else { + tcp_up = connect_fail_limit; + } +} + +// TODO: Require callers to detail failures instead of using one (and often +// misleading!) "connection failed" phrase for all of them. +/// noteFailure() helper for handling failures attributed to this peer +void +CachePeer::noteFailure() +{ + stats.last_connect_failure = squid_curtime; + if (tcp_up > 0) + --tcp_up; + + const auto consideredAliveByAdmin = (stats.logged_state == PEER_ALIVE); + const auto level = consideredAliveByAdmin ? DBG_IMPORTANT : 2; + debugs(15, level, "ERROR: Connection to " << this->host << "/" << this->http_port << " failed"); + + if (consideredAliveByAdmin) { + if (!tcp_up) { + debugs(15, DBG_IMPORTANT, "Detected DEAD " << neighborTypeStr(this) << ": " << name); + stats.logged_state = PEER_DEAD; + } else { + debugs(15, 2, "additional failures needed to mark this cache_peer DEAD: " << tcp_up); + } + } else { + assert(!tcp_up); + debugs(15, 2, "cache_peer " << this->host << "/" << this->http_port << " is still DEAD"); + } +} + + diff --git a/src/CachePeer.h b/src/CachePeer.h index edeb048..be753b9 100644 --- a/src/CachePeer.h +++ b/src/CachePeer.h @@ -12,6 +12,7 @@ #include "acl/forward.h" #include "base/CbcPointer.h" #include "enums.h" +#include "http/StatusCode.h" #include "icp_opcode.h" #include "ip/Address.h" #include "security/PeerOptions.h" @@ -32,6 +33,12 @@ public: CachePeer() = default; ~CachePeer(); + /// reacts to a successful establishment of a connection to this cache_peer + void noteSuccess(); + + /// reacts to a failed attempt to establish a connection to this cache_peer + void noteFailure(); + u_int index = 0; char *name = nullptr; char *host = nullptr; @@ -190,7 +197,28 @@ public: int front_end_https = 0; ///< 0 - off, 1 - on, 2 - auto int connection_auth = 2; ///< 0 - off, 1 - on, 2 - auto + +private: + void countFailure(); }; +/// reacts to a successful establishment of a connection to an origin server or cache_peer +/// \param peer nil if Squid established a connection to an origin server +inline void +NoteOutgoingConnectionSuccess(CachePeer * const peer) +{ + if (peer) + peer->noteSuccess(); +} + +/// reacts to a failed attempt to establish a connection to an origin server or cache_peer +/// \param peer nil if the connection is to an origin server +inline void +NoteOutgoingConnectionFailure(CachePeer * const peer) +{ + if (peer) + peer->noteFailure(); +} + #endif /* SQUID_CACHEPEER_H_ */ diff --git a/src/FwdState.cc b/src/FwdState.cc index c089f82..254b4d5 100644 --- a/src/FwdState.cc +++ b/src/FwdState.cc @@ -1083,8 +1083,7 @@ FwdState::successfullyConnectedToPeer(const Comm::ConnectionPointer &conn) CallJobHere1(17, 4, request->clientConnectionManager, ConnStateData, ConnStateData::notePeerConnection, serverConnection()); - if (serverConnection()->getPeer()) - peerConnectSucceded(serverConnection()->getPeer()); + NoteOutgoingConnectionSuccess(serverConnection()->getPeer()); dispatch(); } diff --git a/src/FwdState.h b/src/FwdState.h index 0bcfafd..dea29d5 100644 --- a/src/FwdState.h +++ b/src/FwdState.h @@ -30,7 +30,6 @@ class AccessLogEntry; typedef RefCount AccessLogEntryPointer; -class ErrorState; class HttpRequest; class PconnPool; class ResolvedPeers; diff --git a/src/HappyConnOpener.cc b/src/HappyConnOpener.cc index 6d83ff1..009e3f7 100644 --- a/src/HappyConnOpener.cc +++ b/src/HappyConnOpener.cc @@ -617,8 +617,6 @@ HappyConnOpener::handleConnOpenerAnswer(Attempt &attempt, const CommConnectCbPar } debugs(17, 8, what << " failed: " << params.conn); - if (const auto peer = params.conn->getPeer()) - peerConnectFailed(peer); // remember the last failure (we forward it if we cannot connect anywhere) lastFailedConnection = handledPath; @@ -627,6 +625,8 @@ HappyConnOpener::handleConnOpenerAnswer(Attempt &attempt, const CommConnectCbPar lastError = makeError(ERR_CONNECT_FAIL); lastError->xerrno = params.xerrno; + NoteOutgoingConnectionFailure(params.conn->getPeer()); + if (spareWaiting) updateSpareWaitAfterPrimeFailure(); diff --git a/src/PeerPoolMgr.cc b/src/PeerPoolMgr.cc index 7423dd6..bd73cb5 100644 --- a/src/PeerPoolMgr.cc +++ b/src/PeerPoolMgr.cc @@ -99,7 +99,7 @@ PeerPoolMgr::handleOpenedConnection(const CommConnectCbParams ¶ms) } if (params.flag != Comm::OK) { - peerConnectFailed(peer); + NoteOutgoingConnectionFailure(peer); checkpoint("conn opening failure"); // may retry return; } @@ -148,7 +148,7 @@ PeerPoolMgr::handleSecuredPeer(Security::EncryptorAnswer &answer) assert(!answer.tunneled); if (answer.error.get()) { assert(!answer.conn); - // PeerConnector calls peerConnectFailed() for us; + // PeerConnector calls NoteOutgoingConnectionFailure() for us checkpoint("conn securing failure"); // may retry return; } diff --git a/src/clients/HttpTunneler.cc b/src/clients/HttpTunneler.cc index 67a52e6..84c090a 100644 --- a/src/clients/HttpTunneler.cc +++ b/src/clients/HttpTunneler.cc @@ -389,8 +389,9 @@ void Http::Tunneler::countFailingConnection() { assert(connection); - if (const auto p = connection->getPeer()) - peerConnectFailed(p); + // No NoteOutgoingConnectionFailure(connection->getPeer()) call here because + // we do not blame cache_peer for CONNECT failures (on top of a successfully + // established connection to that cache_peer). if (noteFwdPconnUse && connection->isOpen()) fwdPconnPool->noteUses(fd_table[connection->fd].pconn.uses); } diff --git a/src/error/forward.h b/src/error/forward.h index c105a66..478aac0 100644 --- a/src/error/forward.h +++ b/src/error/forward.h @@ -90,6 +90,7 @@ typedef enum { class Error; class ErrorDetail; +class ErrorState; typedef RefCount ErrorDetailPointer; diff --git a/src/http/StatusCode.h b/src/http/StatusCode.h index d9f3620..c093017 100644 --- a/src/http/StatusCode.h +++ b/src/http/StatusCode.h @@ -90,6 +90,8 @@ typedef enum { const char *StatusCodeString(const Http::StatusCode status); /// whether this is an informational 1xx response status code inline bool Is1xx(const int sc) { return scContinue <= sc && sc < scOkay; } +/// whether this is a client error 4xx response status code +inline bool Is4xx(const int sc) { return scBadRequest <= sc && sc < scInternalServerError; } /// whether this response status code prohibits sending Content-Length inline bool ProhibitsContentLength(const StatusCode sc) { return sc == scNoContent || Is1xx(sc); } diff --git a/src/neighbors.cc b/src/neighbors.cc index a68d53d..b36b1d0 100644 --- a/src/neighbors.cc +++ b/src/neighbors.cc @@ -452,9 +452,6 @@ peerClearRR() } } -/** - * Perform all actions when a CachePeer is detected revived. - */ void peerAlive(CachePeer *p) { @@ -468,6 +465,10 @@ peerAlive(CachePeer *p) p->stats.last_reply = squid_curtime; p->stats.probe_start = 0; + + // TODO: Remove or explain how we could detect an alive peer without IP addresses + if (!p->n_addresses) + ipcache_nbgethostbyname(p->host, peerDNSConfigure, p); } CachePeer * @@ -1275,45 +1276,6 @@ peerRefreshDNS(void *data) eventAddIsh("peerRefreshDNS", peerRefreshDNS, NULL, 3600.0, 1); } -static void -peerConnectFailedSilent(CachePeer * p) -{ - p->stats.last_connect_failure = squid_curtime; - - if (!p->tcp_up) { - debugs(15, 2, "TCP connection to " << p->host << "/" << p->http_port << - " dead"); - return; - } - - -- p->tcp_up; - - if (!p->tcp_up) { - debugs(15, DBG_IMPORTANT, "Detected DEAD " << neighborTypeStr(p) << ": " << p->name); - p->stats.logged_state = PEER_DEAD; - } -} - -void -peerConnectFailed(CachePeer *p) -{ - debugs(15, DBG_IMPORTANT, "TCP connection to " << p->host << "/" << p->http_port << " failed"); - peerConnectFailedSilent(p); -} - -void -peerConnectSucceded(CachePeer * p) -{ - if (!p->tcp_up) { - debugs(15, 2, "TCP connection to " << p->host << "/" << p->http_port << " succeeded"); - p->tcp_up = p->connect_fail_limit; // NP: so peerAlive(p) works properly. - peerAlive(p); - if (!p->n_addresses) - ipcache_nbgethostbyname(p->host, peerDNSConfigure, p); - } else - p->tcp_up = p->connect_fail_limit; -} - /// whether new TCP probes are currently banned static bool peerProbeIsBusy(const CachePeer *p) @@ -1365,11 +1327,10 @@ peerProbeConnectDone(const Comm::ConnectionPointer &conn, Comm::Flag status, int { CachePeer *p = (CachePeer*)data; - if (status == Comm::OK) { - peerConnectSucceded(p); - } else { - peerConnectFailedSilent(p); - } + if (status == Comm::OK) + p->noteSuccess(); + else + p->noteFailure(); -- p->testing_now; conn->close(); diff --git a/src/neighbors.h b/src/neighbors.h index 95f0928..870943d 100644 --- a/src/neighbors.h +++ b/src/neighbors.h @@ -49,6 +49,12 @@ CachePeer *getRoundRobinParent(PeerSelector*); CachePeer *getWeightedRoundRobinParent(PeerSelector*); void peerClearRRStart(void); void peerClearRR(void); + +// TODO: Move, together with its many dependencies and callers, into CachePeer. +/// Updates protocol-agnostic CachePeer state after an indication of a +/// successful contact with the given cache_peer. +void peerAlive(CachePeer *); + lookup_t peerDigestLookup(CachePeer * p, PeerSelector *); CachePeer *neighborsDigestSelect(PeerSelector *); void peerNoteDigestLookup(HttpRequest * request, CachePeer * p, lookup_t lookup); @@ -56,8 +62,6 @@ void peerNoteDigestGone(CachePeer * p); int neighborUp(const CachePeer * e); const char *neighborTypeStr(const CachePeer * e); peer_t neighborType(const CachePeer *, const AnyP::Uri &); -void peerConnectFailed(CachePeer *); -void peerConnectSucceded(CachePeer *); void dump_peer_options(StoreEntry *, CachePeer *); int peerHTTPOkay(const CachePeer *, PeerSelector *); diff --git a/src/security/BlindPeerConnector.cc b/src/security/BlindPeerConnector.cc index d0d1c4a..8c12843 100644 --- a/src/security/BlindPeerConnector.cc +++ b/src/security/BlindPeerConnector.cc @@ -70,13 +70,13 @@ Security::BlindPeerConnector::noteNegotiationDone(ErrorState *error) if (error) { debugs(83, 5, "error=" << (void*)error); - // XXX: forward.cc calls peerConnectSucceeded() after an OK TCP connect but - // we call peerConnectFailed() if SSL failed afterwards. Is that OK? - // It is not clear whether we should call peerConnectSucceeded/Failed() + // XXX: FwdState calls NoteOutgoingConnectionSuccess() after an OK TCP connect, but + // we call noteFailure() if SSL failed afterwards. Is that OK? + // It is not clear whether we should call noteSuccess()/noteFailure()/etc. // based on TCP results, SSL results, or both. And the code is probably not // consistent in this aspect across tunnelling and forwarding modules. if (peer && peer->secure.encryptTransport) - peerConnectFailed(peer); + peer->noteFailure(); return; } diff --git a/src/security/BlindPeerConnector.h b/src/security/BlindPeerConnector.h index 37e2fc5..3553859 100644 --- a/src/security/BlindPeerConnector.h +++ b/src/security/BlindPeerConnector.h @@ -42,8 +42,8 @@ public: /// Return the configured TLS context object virtual Security::ContextPointer getTlsContext(); - /// On error calls peerConnectFailed(). - /// On success store the used TLS session for later use. + /// On success, stores the used TLS session for later use. + /// On error, informs the peer. virtual void noteNegotiationDone(ErrorState *); }; diff --git a/src/security/PeerConnector.cc b/src/security/PeerConnector.cc index 494edab..0b179bb 100644 --- a/src/security/PeerConnector.cc +++ b/src/security/PeerConnector.cc @@ -10,6 +10,7 @@ #include "squid.h" #include "acl/FilledChecklist.h" +#include "CachePeer.h" #include "comm/Loops.h" #include "comm/Read.h" #include "Downloader.h" @@ -92,15 +93,17 @@ Security::PeerConnector::commCloseHandler(const CommCloseCbParams ¶ms) debugs(83, 5, "FD " << params.fd << ", Security::PeerConnector=" << params.data); closeHandler = nullptr; + + const auto err = new ErrorState(ERR_SECURE_CONNECT_FAIL, Http::scServiceUnavailable, request.getRaw(), al); + static const auto d = MakeNamedErrorDetail("TLS_CONNECT_CLOSE"); + err->detailError(d); + if (serverConn) { countFailingConnection(); serverConn->noteClosure(); serverConn = nullptr; } - const auto err = new ErrorState(ERR_SECURE_CONNECT_FAIL, Http::scServiceUnavailable, request.getRaw(), al); - static const auto d = MakeNamedErrorDetail("TLS_CONNECT_CLOSE"); - err->detailError(d); bail(err); } @@ -510,8 +513,7 @@ void Security::PeerConnector::countFailingConnection() { assert(serverConn); - if (const auto p = serverConn->getPeer()) - peerConnectFailed(p); + NoteOutgoingConnectionFailure(serverConn->getPeer()); // TODO: Calling PconnPool::noteUses() should not be our responsibility. if (noteFwdPconnUse && serverConn->isOpen()) fwdPconnPool->noteUses(fd_table[serverConn->fd].pconn.uses); diff --git a/src/security/PeerConnector.h b/src/security/PeerConnector.h index 10700d7..a913b25 100644 --- a/src/security/PeerConnector.h +++ b/src/security/PeerConnector.h @@ -24,7 +24,6 @@ #include #include -class ErrorState; class Downloader; class AccessLogEntry; typedef RefCount AccessLogEntryPointer;