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.

diff --git a/src/HappyConnOpener.cc b/src/HappyConnOpener.cc
index baf3122c18779299dda73eff593550e37c6d565e..2e3db9c76c58cf0999d4d579581d753029ce6444 100644 (file)
--- 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<HappyConnOpener, CommConnectCbParams> 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 &params)
+HappyConnOpener::notePrimeConnectDone(const CommConnectCbParams &params)
 {
-    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 &params)
+{
+    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 &params, 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 &params)
     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 162dd9e5fc4295c16a56d0b53bfcc45b90a056a7..e46f2527f2df5a1b68d42cbab44f16f06532d290 100644 (file)
--- 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<bool>(path); }
 
         /// reacts to a natural attempt completion (successful or otherwise)
@@ -166,6 +171,9 @@ private:
 
         PeerConnectionPointer path; ///< the destination we are connecting to
         JobWait<Comm::ConnOpener> 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 745669706996b27710d2487d80aaf6f83eafe439..bd950715d8cfa2f7078900f21f12a55b42529361 100644 (file)
--- a/src/PeerPoolMgr.cc
+++ b/src/PeerPoolMgr.cc
@@ -99,9 +99,6 @@ PeerPoolMgr::handleOpenedConnection(const CommConnectCbParams &params)
     }
 
     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 6a27bb683fe0b5e53b61d02388f6cf02fb1b63d9..fec4b12c19e7e2f2a63b9efa66ed085d1e023ff3 100644 (file)
--- 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 9344308f5c2b4c996a148e8d0ab8a0258ace31cd..5625f16065eed87c2958eca0d8b18b63c0b2a0d2 100644 (file)
--- 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 &params = GetCommParams<Params>(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 41e751c33a5c59e3821cdddab9e16bcdefe8395a..d20ba3420e4903bb266880f258f244abf092b941 100644 (file)
--- 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 1f1c706513bcf5429777744556e66eaae0668421..72e51b84d7edcfeaf7ad1c7ba67b04ace18e9976 100644 (file)
--- 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 d796e9ed1c1d3575c9b819ae7252a66f786fb71d..df886dd705dc3417edc96e161ee1a512d547b714 100644 (file)
--- 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 641ced5fd994da044da877d13f85d5aab2202b0f..db4078800670caec758dcd160fdece2e0c669768 100644 (file)
--- 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<Server, CommConnectCbParams> Dialer;
     AsyncCall::Pointer callback = JobCallback(17, 3, Dialer, this, Ftp::Server::connectedForData);
@@ -1706,15 +1704,18 @@ Ftp::Server::connectedForData(const CommConnectCbParams &params)
     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");
     }