From 21f1082fe26158fe613de12d0548e9d4bb7c9ace Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Sun, 1 Aug 2021 17:20:48 -0400 Subject: [PATCH] Adjusted ConnOpener API to fix several problems 1. Many ConnOpener users are witten to keep a ConnectionPointer to the destination given to ConnOpener. This means that their connection magically opens when ConnOpener successfully connects, before ConnOpener has a chance to notify the user about the changes. Having multiple concurrent connection owners is always dangerous, and the user cannot even have a close handler registered for its now-open connection. When something happens to ConnOpener or its answer, the user job may be in trouble. ConnOpener now clones the destination parameter, refusing to tie ConnOpener connection to the ConnOpener creator connection. This addresses the concern, but requires adjustment 2. 2. Refactored ConnOpener users to stop assuming that the answer contains a pointer to their connection object. After adjustment 1 above, it does not. HappyConnOpener relied on that assumption quite a bit so we had to refactor to use two custom callback methods instead of one with a complicated if-statement distinguishing prime from spare attempts. This refactoring is an overall improvement because it simplifies the code. Other ConnOpener users just needed to remove a few no longer valid paranoid assertions/Musts. 3. ConnOpener users were forced to remember to close params.conn when processing negative answers. Some, naturally, forgot, triggering warnings about orphaned connections (e.g., Ident and TcpLogger). ConnOpener now closes its open connection before sending a negative answer. 4. ConnOpener would trigger orphan connection warnings if the job ended after opening the connection but without supplying the connection to the requestor (e.g., because the requestor has gone away). Now, ConnOpener explicitly closes its open connection if it has not been sent to the requestor. Also fixed Comm::ConnOpener::cleanFd() debugging that was incorrectly saying that the method closes the temporary descriptor. Also added Comm::Connection::cloneDestinationDetails() debugging to simplify tracking dependencies between half-baked Connection objects carrying destination/flags/other metadata and open Connection objects created by ConnOpener using that metadata (which are later delivered to ConnOpener users and, in some cases, replace those half-baked connections mentioned earlier. Long-term, we need to find a better way to express these and other Connection states/stages than comments and debugging messages. --- src/HappyConnOpener.cc | 61 +++++++++++++++++++--------------- src/HappyConnOpener.h | 12 ++++++- src/PeerPoolMgr.cc | 3 -- src/adaptation/icap/Xaction.cc | 14 ++++++-- src/comm/ConnOpener.cc | 21 ++++++++++-- src/comm/ConnOpener.h | 5 ++- src/comm/Connection.cc | 1 + src/ident/Ident.cc | 16 ++++++--- src/servers/FtpServer.cc | 13 ++++---- 9 files changed, 98 insertions(+), 48 deletions(-) diff --git a/src/HappyConnOpener.cc b/src/HappyConnOpener.cc index baf3122c18..2e3db9c76c 100644 --- a/src/HappyConnOpener.cc +++ b/src/HappyConnOpener.cc @@ -331,6 +331,8 @@ HappyConnOpener::HappyConnOpener(const ResolvedPeers::Pointer &dests, const Asyn fwdStart(aFwdStart), callback_(aCall), destinations(dests), + prime(&HappyConnOpener::notePrimeConnectDone, "HappyConnOpener::notePrimeConnectDone"), + spare(&HappyConnOpener::noteSpareConnectDone, "HappyConnOpener::noteSpareConnectDone"), ale(anAle), cause(request), n_tries(tries) @@ -571,43 +573,47 @@ HappyConnOpener::openFreshConnection(Attempt &attempt, PeerConnectionPointer &de ++n_tries; typedef CommCbMemFunT Dialer; - AsyncCall::Pointer callConnect = JobCallback(48, 5, Dialer, this, HappyConnOpener::connectDone); + AsyncCall::Pointer callConnect = asyncCall(48, 5, attempt.callbackMethodName, + Dialer(this, attempt.callbackMethod)); const time_t connTimeout = dest->connectTimeout(fwdStart); Comm::ConnOpener *cs = new Comm::ConnOpener(dest, callConnect, connTimeout); if (!dest->getPeer()) cs->setHost(host_); - // XXX: Do not co-own attempt.path with ConnOpener. - attempt.path = dest; attempt.connWait.start(cs, callConnect); AsyncJob::Start(cs); } -/// called by Comm::ConnOpener objects after a prime or spare connection attempt -/// completes (successfully or not) +/// Comm::ConnOpener callback for the prime connection attempt void -HappyConnOpener::connectDone(const CommConnectCbParams ¶ms) +HappyConnOpener::notePrimeConnectDone(const CommConnectCbParams ¶ms) { - Must(params.conn); - const bool itWasPrime = (params.conn == prime.path); - 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(); - gotSpareAllowance = false; - } + handleConnOpenerAnswer(prime, params, "new prime connection"); +} + +/// Comm::ConnOpener callback for the spare connection attempt +void +HappyConnOpener::noteSpareConnectDone(const CommConnectCbParams ¶ms) +{ + if (gotSpareAllowance) { + TheSpareAllowanceGiver.jobUsedAllowance(); + gotSpareAllowance = false; } + handleConnOpenerAnswer(spare, params, "new spare connection"); +} + +/// prime/spare-agnostic processing of a Comm::ConnOpener result +void +HappyConnOpener::handleConnOpenerAnswer(Attempt &attempt, const CommConnectCbParams ¶ms, const char *what) +{ + Must(params.conn); + + // finalize the previously selected path before attempt.finish() forgets it + auto handledPath = attempt.path; + handledPath.finalize(params.conn); // closed on errors + attempt.finish(); - const char *what = itWasPrime ? "new prime connection" : "new spare connection"; if (params.flag == Comm::OK) { sendSuccess(handledPath, false, what); return; @@ -616,7 +622,6 @@ HappyConnOpener::connectDone(const CommConnectCbParams ¶ms) debugs(17, 8, what << " failed: " << params.conn); if (const auto peer = params.conn->getPeer()) peerConnectFailed(peer); - params.conn->close(); // TODO: Comm::ConnOpener should do this instead. // remember the last failure (we forward it if we cannot connect anywhere) lastFailedConnection = handledPath; @@ -732,8 +737,6 @@ HappyConnOpener::checkForNewConnection() if (!destinations->empty()) { if (!currentPeer) { auto newPrime = destinations->extractFront(); - // XXX: Do not co-own currentPeer Connection with ConnOpener - // (which is activated via startConnecting() below). currentPeer = newPrime; Must(currentPeer); debugs(17, 7, "new peer " << *currentPeer); @@ -894,6 +897,12 @@ HappyConnOpener::ranOutOfTimeOrAttempts() const return false; } +HappyConnOpener::Attempt::Attempt(const CallbackMethod method, const char *methodName): + callbackMethod(method), + callbackMethodName(methodName) +{ +} + void HappyConnOpener::Attempt::finish() { diff --git a/src/HappyConnOpener.h b/src/HappyConnOpener.h index 162dd9e5fc..e46f2527f2 100644 --- a/src/HappyConnOpener.h +++ b/src/HappyConnOpener.h @@ -156,6 +156,11 @@ private: /// a connection opening attempt in progress (or falsy) class Attempt { public: + /// HappyConnOpener method implementing a ConnOpener callback + using CallbackMethod = void (HappyConnOpener::*)(const CommConnectCbParams &); + + Attempt(const CallbackMethod method, const char *methodName); + explicit operator bool() const { return static_cast(path); } /// reacts to a natural attempt completion (successful or otherwise) @@ -166,6 +171,9 @@ private: PeerConnectionPointer path; ///< the destination we are connecting to JobWait connWait; ///< establishes a transport connection + + const CallbackMethod callbackMethod; ///< ConnOpener calls this method + const char * const callbackMethodName; ///< for callbackMethod debugging }; friend std::ostream &operator <<(std::ostream &, const Attempt &); @@ -186,7 +194,9 @@ private: void openFreshConnection(Attempt &, PeerConnectionPointer &); bool reuseOldConnection(PeerConnectionPointer &); - void connectDone(const CommConnectCbParams &); + void notePrimeConnectDone(const CommConnectCbParams &); + void noteSpareConnectDone(const CommConnectCbParams &); + void handleConnOpenerAnswer(Attempt &, const CommConnectCbParams &, const char *connDescription); void checkForNewConnection(); diff --git a/src/PeerPoolMgr.cc b/src/PeerPoolMgr.cc index 7456697069..bd950715d8 100644 --- a/src/PeerPoolMgr.cc +++ b/src/PeerPoolMgr.cc @@ -99,9 +99,6 @@ PeerPoolMgr::handleOpenedConnection(const CommConnectCbParams ¶ms) } if (params.flag != Comm::OK) { - /* it might have been a timeout with a partially open link */ - if (params.conn != NULL) - params.conn->close(); peerConnectFailed(peer); checkpoint("conn opening failure"); // may retry return; diff --git a/src/adaptation/icap/Xaction.cc b/src/adaptation/icap/Xaction.cc index 6a27bb683f..fec4b12c19 100644 --- a/src/adaptation/icap/Xaction.cc +++ b/src/adaptation/icap/Xaction.cc @@ -268,10 +268,18 @@ void Adaptation::Icap::Xaction::noteCommConnected(const CommConnectCbParams &io) return; } - if (io.flag != Comm::OK) + if (io.flag != Comm::OK) { dieOnConnectionFailure(); // throws - else - successfullyConnected(); + return; + } + + // Finalize the details and start owning the supplied connection, possibly + // prematurely -- see XXX in successfullyConnected(). + assert(io.conn); + assert(connection); + assert(!connection->isOpen()); + connection = io.conn; + successfullyConnected(); } /// called when successfully connected to an ICAP service diff --git a/src/comm/ConnOpener.cc b/src/comm/ConnOpener.cc index 9344308f5c..5625f16065 100644 --- a/src/comm/ConnOpener.cc +++ b/src/comm/ConnOpener.cc @@ -34,7 +34,7 @@ Comm::ConnOpener::ConnOpener(const Comm::ConnectionPointer &c, const AsyncCall:: AsyncJob("Comm::ConnOpener"), host_(NULL), temporaryFd_(-1), - conn_(c), + conn_(c->cloneDestinationDetails()), callback_(handler), totalTries_(0), failRetries_(0), @@ -78,6 +78,10 @@ Comm::ConnOpener::swanSong() if (temporaryFd_ >= 0) closeFd(); + // did we abort while owning an open connection? + if (conn_ && conn_->isOpen()) + conn_->close(); + // did we abort while waiting between retries? if (calls_.sleep_) cancelSleep(); @@ -131,9 +135,21 @@ Comm::ConnOpener::sendAnswer(Comm::Flag errFlag, int xerrno, const char *why) " [" << callback_->id << ']' ); // TODO save the pconn to the pconnPool ? } else { + assert(conn_); + + // free resources earlier and simplify recipients + if (errFlag != Comm::OK) + conn_->close(); // may not be opened + else + assert(conn_->isOpen()); + + // XXX: CommConnectCbParams imply syncWithComm(). Our recipients + // need a callback even if conn is closing. TODO: Do not reuse + // CommConnectCbParams; implement a different syncing logic. typedef CommConnectCbParams Params; Params ¶ms = GetCommParams(callback_); params.conn = conn_; + conn_ = nullptr; // release ownership; prevent closure by us params.flag = errFlag; params.xerrno = xerrno; ScheduleCallHere(callback_); @@ -152,7 +168,7 @@ Comm::ConnOpener::sendAnswer(Comm::Flag errFlag, int xerrno, const char *why) void Comm::ConnOpener::cleanFd() { - debugs(5, 4, HERE << conn_ << " closing temp FD " << temporaryFd_); + debugs(5, 4, conn_ << "; temp FD " << temporaryFd_); Must(temporaryFd_ >= 0); fde &f = fd_table[temporaryFd_]; @@ -258,6 +274,7 @@ bool Comm::ConnOpener::createFd() { Must(temporaryFd_ < 0); + assert(conn_); // our initiators signal abort by cancelling their callbacks if (callback_ == NULL || callback_->canceled()) diff --git a/src/comm/ConnOpener.h b/src/comm/ConnOpener.h index 41e751c33a..d20ba3420e 100644 --- a/src/comm/ConnOpener.h +++ b/src/comm/ConnOpener.h @@ -19,9 +19,8 @@ namespace Comm { -/** - * Async-opener of a Comm connection. - */ +/// Asynchronously opens a TCP connection. Returns CommConnectCbParams: either +/// Comm::OK with an open connection or another Comm::Flag with a closed one. class ConnOpener : public AsyncJob { CBDATA_CLASS(ConnOpener); diff --git a/src/comm/Connection.cc b/src/comm/Connection.cc index 1f1c706513..72e51b84d7 100644 --- a/src/comm/Connection.cc +++ b/src/comm/Connection.cc @@ -68,6 +68,7 @@ Comm::Connection::cloneDestinationDetails() const c->peerType = peerType; c->flags = flags; c->peer_ = cbdataReference(getPeer()); + debugs(5, 5, this << " made " << c); assert(!c->isOpen()); return c; } diff --git a/src/ident/Ident.cc b/src/ident/Ident.cc index d796e9ed1c..df886dd705 100644 --- a/src/ident/Ident.cc +++ b/src/ident/Ident.cc @@ -141,6 +141,16 @@ Ident::ConnectDone(const Comm::ConnectionPointer &conn, Comm::Flag status, int, IdentStateData *state = (IdentStateData *)data; state->connWait.finish(); + // Finalize the details and start owning the supplied connection (so that it + // is not orphaned if this function bails early). As a (tiny) optimization + // or perhaps just diff minimization, the close handler is added later, when + // we know we are not bailing. This delay is safe because + // comm_remove_close_handler() forgives missing handlers. + assert(conn); // but may be closed + assert(state->conn); + assert(!state->conn->isOpen()); + state->conn = conn; + if (status != Comm::OK) { if (status == Comm::TIMEOUT) debugs(30, 3, "IDENT connection timeout to " << state->conn->remote); @@ -162,8 +172,8 @@ Ident::ConnectDone(const Comm::ConnectionPointer &conn, Comm::Flag status, int, return; } - assert(conn != NULL && conn == state->conn); - comm_add_close_handler(conn->fd, Ident::Close, state); + assert(state->conn->isOpen()); + comm_add_close_handler(state->conn->fd, Ident::Close, state); AsyncCall::Pointer writeCall = commCbCall(5,4, "Ident::WriteFeedback", CommIoCbPtrFun(Ident::WriteFeedback, state)); @@ -271,8 +281,6 @@ Ident::Start(const Comm::ConnectionPointer &conn, IDCB * callback, void *data) state = new IdentStateData; state->hash.key = xstrdup(key); - // XXX: Do not co-own state->conn Connection with ConnOpener. - // copy the conn details. We do not want the original FD to be re-used by IDENT. state->conn = conn->cloneIdentDetails(); // NP: use random port for secure outbound to IDENT_PORT diff --git a/src/servers/FtpServer.cc b/src/servers/FtpServer.cc index 641ced5fd9..db40788006 100644 --- a/src/servers/FtpServer.cc +++ b/src/servers/FtpServer.cc @@ -1676,8 +1676,6 @@ Ftp::Server::checkDataConnPre() return false; } - // XXX: Do not co-own dataConn with ConnOpener. - // active transfer: open a data connection from Squid to client typedef CommCbMemFunT Dialer; AsyncCall::Pointer callback = JobCallback(17, 3, Dialer, this, Ftp::Server::connectedForData); @@ -1706,15 +1704,18 @@ Ftp::Server::connectedForData(const CommConnectCbParams ¶ms) dataConnWait.finish(); if (params.flag != Comm::OK) { - /* it might have been a timeout with a partially open link */ - if (params.conn != NULL) - params.conn->close(); setReply(425, "Cannot open data connection."); Http::StreamPointer context = pipeline.front(); Must(context->http); Must(context->http->storeEntry() != NULL); } else { - Must(dataConn == params.conn); + // Finalize the details and start owning the supplied connection. + assert(params.conn); + assert(dataConn); + assert(!dataConn->isOpen()); + dataConn = params.conn; + // XXX: Missing comm_add_close_handler() to track external closures. + Must(Comm::IsConnOpen(params.conn)); fd_note(params.conn->fd, "active client ftp data"); } -- 2.47.2