From: Eduard Bagdasaryan Date: Fri, 26 Jun 2020 20:06:29 +0000 (+0000) Subject: Bug 5048: ResolvedPeers.cc:35: "found != paths_.end()" assertion (#680) X-Git-Tag: 4.15-20210522-snapshot~86 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=9b7992d91b8b119707afd9f8dc4976bb9e209f2e;p=thirdparty%2Fsquid.git Bug 5048: ResolvedPeers.cc:35: "found != paths_.end()" assertion (#680) This bug was caused by a4d576d. ResolvedPeers::retryPath() used connection object pointers to find the original path location under the incorrect assumption that the Connection pointers never change. That assumption was wrong for persistent connections: ResolvedPeers stores a half-baked Connection object rather than the corresponding open Connection object that the unfortunate caller got from fwdPconnPool and passed to ResolvedPeers::retryPath(). While working on a fix, we discovered a second reason why the pointer comparison was a bad idea (and why a simpler fix of comparing addresses rather than pointers was also a bad idea): The peer selection code promises to deliver unique destinations, but we now suspect that, under presumably rare circumstances, it may deliver duplicates. This broken promise is now an out-of-scope XXX in PeerSelector::addSelection(). Squid now eliminates the search (and the previously documented concern about its slow speed) by remembering Connection's position in the destination array. The position is remembered in a smart Connection pointer (compatible with Comm::ConnectionPointer) so that the rest of the code (that does not really care about all this) is preserved largely unchanged. Most changes are just renaming the pointer type. Also freshened a FwdState call and comment made stale by 25b0ce4. Also marked an out-of-scope problem in maybeGivePrimeItsChance() caller. --- diff --git a/src/FwdState.cc b/src/FwdState.cc index a992dc5477..c9fa449bcf 100644 --- a/src/FwdState.cc +++ b/src/FwdState.cc @@ -484,7 +484,10 @@ FwdState::fail(ErrorState * errorState) if (pconnRace == racePossible) { debugs(17, 5, HERE << "pconn race happened"); pconnRace = raceHappened; - destinations->retryPath(serverConn); + if (destinationReceipt) { + destinations->reinstatePath(destinationReceipt); + destinationReceipt = nullptr; + } } if (ConnStateData *pinned_connection = request->pinnedConnection()) { @@ -505,6 +508,7 @@ FwdState::unregister(Comm::ConnectionPointer &conn) comm_remove_close_handler(conn->fd, closeHandler); closeHandler = NULL; serverConn = NULL; + destinationReceipt = nullptr; } // \deprecated use unregister(Comm::ConnectionPointer &conn) instead @@ -790,6 +794,8 @@ FwdState::advanceDestination(const char *stepDescription, const Comm::Connection void FwdState::noteConnection(HappyConnOpener::Answer &answer) { + assert(!destinationReceipt); + calls.connector = nullptr; connOpener.clear(); @@ -803,14 +809,25 @@ FwdState::noteConnection(HappyConnOpener::Answer &answer) Must(!Comm::IsConnOpen(answer.conn)); answer.error.clear(); // preserve error for errorSendComplete() } else if (!Comm::IsConnOpen(answer.conn) || fd_table[answer.conn->fd].closing()) { + // We do not know exactly why the connection got closed, so we play it + // safe, allowing retries only for persistent (reused) connections + if (answer.reused) { + destinationReceipt = answer.conn; + assert(destinationReceipt); + } syncHierNote(answer.conn, request->url.host()); closePendingConnection(answer.conn, "conn was closed while waiting for noteConnection"); error = new ErrorState(ERR_CANNOT_FORWARD, Http::scServiceUnavailable, request, al); + } else { + assert(!error); + destinationReceipt = answer.conn; + assert(destinationReceipt); + // serverConn remains nil until syncWithServerConn() } if (error) { fail(error); - retryOrBail(); // will notice flags.dont_retry and bail + retryOrBail(); return; } @@ -1001,6 +1018,7 @@ FwdState::syncWithServerConn(const Comm::ConnectionPointer &conn, const char *ho { Must(IsConnOpen(conn)); serverConn = conn; + // no effect on destinationReceipt (which may even be nil here) closeHandler = comm_add_close_handler(serverConn->fd, fwdServerClosedWrapper, this); @@ -1045,6 +1063,7 @@ FwdState::connectStart() err = nullptr; request->clearError(); serverConn = nullptr; + destinationReceipt = nullptr; request->hier.startPeerClock(); @@ -1075,6 +1094,8 @@ FwdState::usePinned() debugs(17, 7, "connection manager: " << connManager); try { + // TODO: Refactor syncWithServerConn() and callers to always set + // serverConn inside that method. serverConn = ConnStateData::BorrowPinnedConnection(request, al); debugs(17, 5, "connection: " << serverConn); } catch (ErrorState * const anErr) { diff --git a/src/FwdState.h b/src/FwdState.h index 0e052f0c33..dafece9382 100644 --- a/src/FwdState.h +++ b/src/FwdState.h @@ -21,6 +21,7 @@ #include "http/StatusCode.h" #include "ip/Address.h" #include "PeerSelectState.h" +#include "ResolvedPeers.h" #include "security/forward.h" #if USE_OPENSSL #include "ssl/support.h" @@ -197,6 +198,7 @@ private: HappyConnOpenerPointer connOpener; ///< current connection opening job ResolvedPeersPointer destinations; ///< paths for forwarding the request Comm::ConnectionPointer serverConn; ///< a successfully opened connection to a server. + PeerConnectionPointer destinationReceipt; ///< peer selection result (or nil) AsyncCall::Pointer closeHandler; ///< The serverConn close handler diff --git a/src/HappyConnOpener.cc b/src/HappyConnOpener.cc index 6e5abd83ac..3bcadf4990 100644 --- a/src/HappyConnOpener.cc +++ b/src/HappyConnOpener.cc @@ -18,7 +18,6 @@ #include "neighbors.h" #include "pconn.h" #include "PeerPoolMgr.h" -#include "ResolvedPeers.h" #include "SquidConfig.h" CBDATA_CLASS_INIT(HappyConnOpener); @@ -455,7 +454,7 @@ HappyConnOpener::makeError(const err_type type) const /// \returns pre-filled Answer if the initiator needs an answer (or nil) HappyConnOpener::Answer * -HappyConnOpener::futureAnswer(const Comm::ConnectionPointer &conn) +HappyConnOpener::futureAnswer(const PeerConnectionPointer &conn) { if (callback_ && !callback_->canceled()) { const auto answer = dynamic_cast(callback_->getDialer()); @@ -469,7 +468,7 @@ HappyConnOpener::futureAnswer(const Comm::ConnectionPointer &conn) /// send a successful result to the initiator (if it still needs an answer) void -HappyConnOpener::sendSuccess(const Comm::ConnectionPointer &conn, bool reused, const char *connKind) +HappyConnOpener::sendSuccess(const PeerConnectionPointer &conn, const bool reused, const char *connKind) { debugs(17, 4, connKind << ": " << conn); if (auto *answer = futureAnswer(conn)) { @@ -485,7 +484,7 @@ void HappyConnOpener::cancelAttempt(Attempt &attempt, const char *reason) { Must(attempt); - destinations->retryPath(attempt.path); // before attempt.cancel() clears path + destinations->reinstatePath(attempt.path); // before attempt.cancel() clears path attempt.cancel(reason); } @@ -514,7 +513,7 @@ HappyConnOpener::noteCandidatesChange() /// starts opening (or reusing) a connection to the given destination void -HappyConnOpener::startConnecting(Attempt &attempt, Comm::ConnectionPointer &dest) +HappyConnOpener::startConnecting(Attempt &attempt, PeerConnectionPointer &dest) { Must(!attempt.path); Must(!attempt.connector); @@ -530,13 +529,14 @@ HappyConnOpener::startConnecting(Attempt &attempt, Comm::ConnectionPointer &dest /// \returns true if and only if reuse was possible /// must be called via startConnecting() bool -HappyConnOpener::reuseOldConnection(const Comm::ConnectionPointer &dest) +HappyConnOpener::reuseOldConnection(PeerConnectionPointer &dest) { assert(allowPconn_); if (const auto pconn = fwdPconnPool->pop(dest, host_, retriable_)) { ++n_tries; - sendSuccess(pconn, true, "reused connection"); + dest.finalize(pconn); + sendSuccess(dest, true, "reused connection"); return true; } @@ -546,7 +546,7 @@ HappyConnOpener::reuseOldConnection(const Comm::ConnectionPointer &dest) /// opens a fresh connection to the given destination /// must be called via startConnecting() void -HappyConnOpener::openFreshConnection(Attempt &attempt, Comm::ConnectionPointer &dest) +HappyConnOpener::openFreshConnection(Attempt &attempt, PeerConnectionPointer &dest) { #if URL_CHECKSUM_DEBUG entry->mem_obj->checkUrlChecksum(); @@ -583,9 +583,12 @@ HappyConnOpener::connectDone(const CommConnectCbParams ¶ms) const bool itWasSpare = (params.conn == spare.path); Must(itWasPrime != itWasSpare); + PeerConnectionPointer handledPath; if (itWasPrime) { + handledPath = prime.path; prime.finish(); } else { + handledPath = spare.path; spare.finish(); if (gotSpareAllowance) { TheSpareAllowanceGiver.jobUsedAllowance(); @@ -595,7 +598,7 @@ HappyConnOpener::connectDone(const CommConnectCbParams ¶ms) const char *what = itWasPrime ? "new prime connection" : "new spare connection"; if (params.flag == Comm::OK) { - sendSuccess(params.conn, false, what); + sendSuccess(handledPath, false, what); return; } @@ -605,7 +608,7 @@ HappyConnOpener::connectDone(const CommConnectCbParams ¶ms) params.conn->close(); // TODO: Comm::ConnOpener should do this instead. // remember the last failure (we forward it if we cannot connect anywhere) - lastFailedConnection = params.conn; + lastFailedConnection = handledPath; delete lastError; lastError = nullptr; // in case makeError() throws lastError = makeError(ERR_CONNECT_FAIL); @@ -717,11 +720,14 @@ HappyConnOpener::checkForNewConnection() // open a new prime and/or a new spare connection if needed if (!destinations->empty()) { if (!currentPeer) { - currentPeer = destinations->extractFront(); + auto newPrime = destinations->extractFront(); + currentPeer = newPrime; Must(currentPeer); debugs(17, 7, "new peer " << *currentPeer); primeStart = current_dtime; - startConnecting(prime, currentPeer); + startConnecting(prime, newPrime); + // TODO: if reuseOldConnection() in startConnecting() above succeeds, + // then we should not get here, and Must(prime) below will fail. maybeGivePrimeItsChance(); Must(prime); // entering state #1.1 } else { diff --git a/src/HappyConnOpener.h b/src/HappyConnOpener.h index c1cb95bda3..f5735d631d 100644 --- a/src/HappyConnOpener.h +++ b/src/HappyConnOpener.h @@ -14,13 +14,13 @@ #include "comm/ConnOpener.h" #include "http/forward.h" #include "log/forward.h" +#include "ResolvedPeers.h" #include class HappyConnOpener; class HappyOrderEnforcer; class JobGapEnforcer; -class ResolvedPeers; typedef RefCount ResolvedPeersPointer; /// A FIFO queue of HappyConnOpener jobs waiting to open a spare connection. @@ -79,7 +79,7 @@ public: /// on success: an open, ready-to-use Squid-to-peer connection /// on failure: either a closed failed Squid-to-peer connection or nil - Comm::ConnectionPointer conn; + PeerConnectionPointer conn; // answer recipients must clear the error member in order to keep its info // XXX: We should refcount ErrorState instead of cbdata-protecting it. @@ -164,7 +164,7 @@ private: /// aborts an in-progress attempt void cancel(const char *reason); - Comm::ConnectionPointer path; ///< the destination we are connecting to + PeerConnectionPointer path; ///< the destination we are connecting to AsyncCall::Pointer connector; ///< our opener callback Comm::ConnOpener::Pointer opener; ///< connects to path and calls us @@ -186,9 +186,9 @@ private: void stopWaitingForSpareAllowance(); void maybeOpenSpareConnection(); - void startConnecting(Attempt &, Comm::ConnectionPointer &); - void openFreshConnection(Attempt &, Comm::ConnectionPointer &); - bool reuseOldConnection(const Comm::ConnectionPointer &); + void startConnecting(Attempt &, PeerConnectionPointer &); + void openFreshConnection(Attempt &, PeerConnectionPointer &); + bool reuseOldConnection(PeerConnectionPointer &); void connectDone(const CommConnectCbParams &); @@ -201,8 +201,8 @@ private: bool ranOutOfTimeOrAttempts() const; ErrorState *makeError(const err_type type) const; - Answer *futureAnswer(const Comm::ConnectionPointer &); - void sendSuccess(const Comm::ConnectionPointer &conn, bool reused, const char *connKind); + Answer *futureAnswer(const PeerConnectionPointer &); + void sendSuccess(const PeerConnectionPointer &conn, bool reused, const char *connKind); void sendFailure(); void cancelAttempt(Attempt &, const char *reason); @@ -230,7 +230,7 @@ private: AccessLogEntryPointer ale; ///< transaction details ErrorState *lastError = nullptr; ///< last problem details (or nil) - Comm::ConnectionPointer lastFailedConnection; ///< nil if none has failed + PeerConnectionPointer lastFailedConnection; ///< nil if none has failed /// whether spare connection attempts disregard happy_eyeballs_* settings bool ignoreSpareRestrictions = false; diff --git a/src/ResolvedPeers.cc b/src/ResolvedPeers.cc index d8f00cb443..6b762e0e90 100644 --- a/src/ResolvedPeers.cc +++ b/src/ResolvedPeers.cc @@ -20,25 +20,20 @@ ResolvedPeers::ResolvedPeers() } void -ResolvedPeers::retryPath(const Comm::ConnectionPointer &path) +ResolvedPeers::reinstatePath(const PeerConnectionPointer &path) { debugs(17, 4, path); assert(path); - // Cannot use pathsToSkip for a faster (reverse) search because there - // may be unavailable paths past pathsToSkip. We could remember - // the last extraction index, but, to completely avoid a linear search, - // extract*() methods should return the path index. - const auto found = std::find_if(paths_.begin(), paths_.end(), - [path](const ResolvedPeerPath &candidate) { - return candidate.connection == path; // (refcounted) pointer comparison - }); - assert(found != paths_.end()); - assert(!found->available); - found->available = true; + + const auto pos = path.position_; + assert(pos < paths_.size()); + + assert(!paths_[pos].available); + paths_[pos].available = true; increaseAvailability(); // if we restored availability of a path that we used to skip, update - const auto pathsToTheLeft = static_cast(found - paths_.begin()); + const auto pathsToTheLeft = pos; if (pathsToTheLeft < pathsToSkip) { pathsToSkip = pathsToTheLeft; } else { @@ -112,14 +107,14 @@ ResolvedPeers::findPeer(const Comm::Connection ¤tPeer) return makeFinding(path, foundNext); } -Comm::ConnectionPointer +PeerConnectionPointer ResolvedPeers::extractFront() { Must(!empty()); return extractFound("first: ", start()); } -Comm::ConnectionPointer +PeerConnectionPointer ResolvedPeers::extractPrime(const Comm::Connection ¤tPeer) { const auto found = findPrime(currentPeer).first; @@ -130,7 +125,7 @@ ResolvedPeers::extractPrime(const Comm::Connection ¤tPeer) return nullptr; } -Comm::ConnectionPointer +PeerConnectionPointer ResolvedPeers::extractSpare(const Comm::Connection ¤tPeer) { const auto found = findSpare(currentPeer).first; @@ -142,7 +137,7 @@ ResolvedPeers::extractSpare(const Comm::Connection ¤tPeer) } /// convenience method to finish a successful extract*() call -Comm::ConnectionPointer +PeerConnectionPointer ResolvedPeers::extractFound(const char *description, const Paths::iterator &found) { auto &path = *found; @@ -156,7 +151,8 @@ ResolvedPeers::extractFound(const char *description, const Paths::iterator &foun while (++pathsToSkip < paths_.size() && !paths_[pathsToSkip].available) {} } - return path.connection; + const auto cleanPath = path.connection->cloneDestinationDetails(); + return PeerConnectionPointer(cleanPath, found - paths_.begin()); } bool @@ -227,3 +223,19 @@ operator <<(std::ostream &os, const ResolvedPeers &peers) return os << peers.size() << (peers.destinationsFinalized ? "" : "+") << " paths"; } +/* PeerConnectionPointer */ + +void +PeerConnectionPointer::print(std::ostream &os) const +{ + // We should see no combinations of a nil connection and a set position + // because assigning nullptr (to our smart pointer) naturally erases both + // fields. We report such unexpected combinations for debugging sake, but do + // not complicate this code to report them beautifully. + + if (connection_) + os << connection_; + + if (position_ != npos) + os << " @" << position_; +} diff --git a/src/ResolvedPeers.h b/src/ResolvedPeers.h index f438c7373f..f4be67ed06 100644 --- a/src/ResolvedPeers.h +++ b/src/ResolvedPeers.h @@ -13,6 +13,7 @@ #include "comm/forward.h" #include +#include #include class ResolvedPeerPath @@ -24,6 +25,8 @@ public: bool available; ///< whether this path may be used (i.e., has not been tried already) }; +class PeerConnectionPointer; + /// cache_peer and origin server addresses (a.k.a. paths) /// selected and resolved by the peering code class ResolvedPeers: public RefCountable @@ -44,19 +47,20 @@ public: /// add a candidate path to try after all the existing paths void addPath(const Comm::ConnectionPointer &); - /// re-inserts the previously extracted address into the same position - void retryPath(const Comm::ConnectionPointer &); + /// makes the previously extracted path available for extraction at its + /// original position + void reinstatePath(const PeerConnectionPointer &); /// extracts and returns the first queued address - Comm::ConnectionPointer extractFront(); + PeerConnectionPointer extractFront(); /// extracts and returns the first same-peer same-family address /// \returns nil if it cannot find the requested address - Comm::ConnectionPointer extractPrime(const Comm::Connection ¤tPeer); + PeerConnectionPointer extractPrime(const Comm::Connection ¤tPeer); /// extracts and returns the first same-peer different-family address /// \returns nil if it cannot find the requested address - Comm::ConnectionPointer extractSpare(const Comm::Connection ¤tPeer); + PeerConnectionPointer extractSpare(const Comm::Connection ¤tPeer); /// whether extractSpare() would return a non-nil path right now bool haveSpare(const Comm::Connection ¤tPeer); @@ -91,7 +95,7 @@ private: Finding findSpare(const Comm::Connection ¤tPeer); Finding findPrime(const Comm::Connection ¤tPeer); Finding findPeer(const Comm::Connection ¤tPeer); - Comm::ConnectionPointer extractFound(const char *description, const Paths::iterator &found); + PeerConnectionPointer extractFound(const char *description, const Paths::iterator &found); Finding makeFinding(const Paths::iterator &found, bool foundOther); bool doneWith(const Finding &findings) const; @@ -110,8 +114,53 @@ private: size_type availablePaths = 0; }; +/// An invasive reference-counting Comm::Connection pointer that also keeps an +/// (optional) ResolvedPeers position for the ResolvedPeers::reinstatePath() usage. +/// Reference counting mechanism is compatible with Comm::ConnectionPointer. +class PeerConnectionPointer +{ +public: + using size_type = ResolvedPeers::size_type; + + PeerConnectionPointer() = default; + PeerConnectionPointer(std::nullptr_t): PeerConnectionPointer() {} ///< implicit nullptr conversion + PeerConnectionPointer(const Comm::ConnectionPointer &conn, const size_type pos): connection_(conn), position_(pos) {} + + /* read-only pointer API; for Connection assignment, see finalize() */ + explicit operator bool() const { return static_cast(connection_); } + Comm::Connection *operator->() const { assert(connection_); return connection_.getRaw(); } + Comm::Connection &operator *() const { assert(connection_); return *connection_; } + + /// convenience conversion to Comm::ConnectionPointer + operator const Comm::ConnectionPointer&() const { return connection_; } + + /// upgrade stored peer selection details with a matching actual connection + void finalize(const Comm::ConnectionPointer &conn) { connection_ = conn; } + + /// debugging dump + void print(std::ostream &) const; + +private: + /// non-existent position for nil connection + static constexpr auto npos = std::numeric_limits::max(); + + /// half-baked, open, failed, or closed Comm::Connection (or nil) + Comm::ConnectionPointer connection_; + + /// ResolvedPeers-maintained membership index (or npos) + size_type position_ = npos; + friend class ResolvedPeers; +}; + /// summarized ResolvedPeers (for debugging) std::ostream &operator <<(std::ostream &, const ResolvedPeers &); +inline std::ostream & +operator <<(std::ostream &os, const PeerConnectionPointer &dest) +{ + dest.print(os); + return os; +} + #endif /* SQUID_RESOLVEDPEERS_H */ diff --git a/src/comm/ConnOpener.cc b/src/comm/ConnOpener.cc index 48ed176320..9344308f5c 100644 --- a/src/comm/ConnOpener.cc +++ b/src/comm/ConnOpener.cc @@ -30,7 +30,7 @@ class CachePeer; CBDATA_NAMESPACED_CLASS_INIT(Comm, ConnOpener); -Comm::ConnOpener::ConnOpener(Comm::ConnectionPointer &c, const AsyncCall::Pointer &handler, time_t ctimeout) : +Comm::ConnOpener::ConnOpener(const Comm::ConnectionPointer &c, const AsyncCall::Pointer &handler, time_t ctimeout) : AsyncJob("Comm::ConnOpener"), host_(NULL), temporaryFd_(-1), diff --git a/src/comm/ConnOpener.h b/src/comm/ConnOpener.h index 37bcb5131f..018bf32b58 100644 --- a/src/comm/ConnOpener.h +++ b/src/comm/ConnOpener.h @@ -33,7 +33,7 @@ public: virtual bool doneAll() const; - ConnOpener(Comm::ConnectionPointer &, const AsyncCall::Pointer &handler, time_t connect_timeout); + ConnOpener(const Comm::ConnectionPointer &, const AsyncCall::Pointer &handler, time_t connect_timeout); ~ConnOpener(); void setHost(const char *); ///< set the hostname note for this connection diff --git a/src/comm/Connection.cc b/src/comm/Connection.cc index c074b39c88..3217d0b71e 100644 --- a/src/comm/Connection.cc +++ b/src/comm/Connection.cc @@ -60,24 +60,25 @@ Comm::Connection::~Connection() } Comm::ConnectionPointer -Comm::Connection::copyDetails() const +Comm::Connection::cloneDestinationDetails() const { - ConnectionPointer c = new Comm::Connection; - + const ConnectionPointer c = new Comm::Connection; c->setAddrs(local, remote); c->peerType = peerType; + c->flags = flags; + c->peer_ = cbdataReference(getPeer()); + assert(!c->isOpen()); + return c; +} + +Comm::ConnectionPointer +Comm::Connection::cloneIdentDetails() const +{ + auto c = cloneDestinationDetails(); c->tos = tos; c->nfmark = nfmark; c->nfConnmark = nfConnmark; - c->flags = flags; c->startTime_ = startTime_; - - // ensure FD is not open in the new copy. - c->fd = -1; - - // ensure we have a cbdata reference to peer_ not a straight ptr copy. - c->peer_ = cbdataReference(getPeer()); - return c; } diff --git a/src/comm/Connection.h b/src/comm/Connection.h index a4f330ca13..202358b2c7 100644 --- a/src/comm/Connection.h +++ b/src/comm/Connection.h @@ -77,10 +77,13 @@ public: /** Clear the connection properties and close any open socket. */ virtual ~Connection(); - /** Copy an existing connections IP and properties. - * This excludes the FD. The new copy will be a closed connection. - */ - ConnectionPointer copyDetails() const; + /// Create a new (closed) IDENT Connection object based on our from-Squid + /// connection properties. + ConnectionPointer cloneIdentDetails() const; + + /// Create a new (closed) Connection object pointing to the same destination + /// as this from-Squid connection. + ConnectionPointer cloneDestinationDetails() const; /// close the still-open connection when its last reference is gone void enterOrphanage() { flags |= COMM_ORPHANED; } @@ -138,10 +141,14 @@ public: virtual std::ostream &detailCodeContext(std::ostream &os) const override; private: - /** These objects may not be exactly duplicated. Use copyDetails() instead. */ + /** These objects may not be exactly duplicated. Use cloneIdentDetails() or + * cloneDestinationDetails() instead. + */ Connection(const Connection &c); - /** These objects may not be exactly duplicated. Use copyDetails() instead. */ + /** These objects may not be exactly duplicated. Use cloneIdentDetails() or + * cloneDestinationDetails() instead. + */ Connection & operator =(const Connection &c); public: diff --git a/src/ident/Ident.cc b/src/ident/Ident.cc index 0b7b761c22..8a2c4ca32f 100644 --- a/src/ident/Ident.cc +++ b/src/ident/Ident.cc @@ -259,7 +259,7 @@ Ident::Start(const Comm::ConnectionPointer &conn, IDCB * callback, void *data) state->hash.key = xstrdup(key); // copy the conn details. We do not want the original FD to be re-used by IDENT. - state->conn = conn->copyDetails(); + state->conn = conn->cloneIdentDetails(); // NP: use random port for secure outbound to IDENT_PORT state->conn->local.port(0); state->conn->remote.port(IDENT_PORT); diff --git a/src/peer_select.cc b/src/peer_select.cc index 49a835ec13..1645f0fa19 100644 --- a/src/peer_select.cc +++ b/src/peer_select.cc @@ -1083,6 +1083,8 @@ PeerSelector::addSelection(CachePeer *peer, const hier_code code) // There can be at most one PINNED destination. // Non-PINNED destinations are uniquely identified by their CachePeer // (even though a DIRECT destination might match a cache_peer address). + // TODO: We may still add duplicates because the same peer could have + // been removed from `servers` already (and given to the requestor). const bool duplicate = (server->code == PINNED) ? (code == PINNED) : (server->_peer == peer); if (duplicate) { diff --git a/src/tests/stub_libcomm.cc b/src/tests/stub_libcomm.cc index 6c921b3727..022a7e8242 100644 --- a/src/tests/stub_libcomm.cc +++ b/src/tests/stub_libcomm.cc @@ -22,7 +22,8 @@ void Comm::AcceptLimiter::kick() STUB #include "comm/Connection.h" Comm::Connection::Connection() STUB Comm::Connection::~Connection() STUB -Comm::ConnectionPointer Comm::Connection::copyDetails() const STUB_RETVAL(NULL) +Comm::ConnectionPointer Comm::Connection::cloneIdentDetails() const STUB_RETVAL(nullptr) +Comm::ConnectionPointer Comm::Connection::cloneDestinationDetails() const STUB_RETVAL(nullptr) void Comm::Connection::close() STUB CachePeer * Comm::Connection::getPeer() const STUB_RETVAL(NULL) void Comm::Connection::setPeer(CachePeer * p) STUB @@ -35,7 +36,7 @@ CBDATA_NAMESPACED_CLASS_INIT(Comm, ConnOpener); bool Comm::ConnOpener::doneAll() const STUB_RETVAL(false) void Comm::ConnOpener::start() STUB void Comm::ConnOpener::swanSong() STUB -Comm::ConnOpener::ConnOpener(Comm::ConnectionPointer &, const AsyncCall::Pointer &, time_t) : AsyncJob("STUB Comm::ConnOpener") STUB +Comm::ConnOpener::ConnOpener(const Comm::ConnectionPointer &, const AsyncCall::Pointer &, time_t) : AsyncJob("STUB Comm::ConnOpener") STUB Comm::ConnOpener::~ConnOpener() STUB void Comm::ConnOpener::setHost(const char *) STUB const char * Comm::ConnOpener::getHost() const STUB_RETVAL(NULL)