Keep Connection and other objects in sync with Comm in closure callbacks

There are lots of small bugs, inconsistencies, and other problems in
Connection closure handlers. It is not clear whether any of those
problems could result in serious runtime errors or leaks. In theory, the
rest of the code could neutralize their negative side effects. However,
even in that case, it was just a matter of time before the next bug will
bite us due to stale Connection::fd and such. These changes themselves
carry elevated risk, but I think we have to do them to get closer to a
reliable code as far as Connection maintenance is concerned; otherwise,
we will keep chasing their deadly side effects.

Long-term, all these manual efforts to keep things in sync should become
unnecessary with the introduction of appropriate Connection ownership
APIs that automatically maintain the corresponding environments (TODO).

Also marked a few newly uncovered bugs in the official code.

diff --git a/src/FwdState.cc b/src/FwdState.cc
index 14718c5768a0e6f87a096048e52abba813d33c9c..3de8e563f3840ad8804b893ca67cb83da2518f98 100644 (file)
--- a/src/FwdState.cc
+++ b/src/FwdState.cc
@@ -662,7 +662,7 @@ static void
 fwdServerClosedWrapper(const CommCloseCbParams &params)
 {
     FwdState *fwd = (FwdState *)params.data;
-    fwd->serverClosed(params.fd);
+    fwd->serverClosed();
 }
 
 /**** PRIVATE *****************************************************************/
@@ -732,14 +732,22 @@ FwdState::checkRetriable()
 }
 
 void
-FwdState::serverClosed(int fd)
+FwdState::serverClosed()
 {
-    // XXX: fd is often -1 here
-    debugs(17, 2, "FD " << fd << " " << entry->url() << " after " <<
-           (fd >= 0 ? fd_table[fd].pconn.uses : -1) << " requests");
-    if (fd >= 0 && serverConnection()->fd == fd)
-        fwdPconnPool->noteUses(fd_table[fd].pconn.uses);
+    // XXX: This method logic attempts to tolerate Connection::close() called
+    // for serverConn earlier, by one of our dispatch()ed jobs. If that happens,
+    // serverConn will already be closed here or, worse, it will already be open
+    // for the next forwarding attempt. The current code prevents us getting
+    // stuck, but the long term solution is to stop sharing serverConn.
+    debugs(17, 2, serverConn);
+    if (Comm::IsConnOpen(serverConn)) {
+        const auto uses = fd_table[serverConn->fd].pconn.uses;
+        debugs(17, 3, "prior uses: " << uses);
+        fwdPconnPool->noteUses(uses); // XXX: May not have come from fwdPconnPool
+        serverConn->noteClosure();
+    }
     serverConn = nullptr;
+    closeHandler = nullptr;
     destinationReceipt = nullptr;
     retryOrBail();
 }

diff --git a/src/FwdState.h b/src/FwdState.h
index 916db122e02f42a43e0d964efb8af057378cde90..4c9a66d9de505f48c70dcc555da593fb331fc9aa 100644 (file)
--- a/src/FwdState.h
+++ b/src/FwdState.h
@@ -88,7 +88,7 @@ public:
     void handleUnregisteredServerEnd();
     int reforward();
     bool reforwardableStatus(const Http::StatusCode s) const;
-    void serverClosed(int fd);
+    void serverClosed();
     void connectStart();
     void connectDone(const Comm::ConnectionPointer & conn, Comm::Flag status, int xerrno);
     bool checkRetry();

diff --git a/src/adaptation/icap/Xaction.cc b/src/adaptation/icap/Xaction.cc
index fb4bba7282f4bd91055cdb3be3b603f9507bd94e..77360fb88d466e4b453ee00e1ecd750a5a6499ed 100644 (file)
--- a/src/adaptation/icap/Xaction.cc
+++ b/src/adaptation/icap/Xaction.cc
@@ -367,6 +367,10 @@ void Adaptation::Icap::Xaction::noteCommTimedout(const CommTimeoutCbParams &)
 // unexpected connection close while talking to the ICAP service
 void Adaptation::Icap::Xaction::noteCommClosed(const CommCloseCbParams &)
 {
+    if (connection) {
+        connection->noteClosure();
+        connection = nullptr;
+    }
     closer = NULL;
     detailError(ERR_DETAIL_ICAP_XACT_CLOSE);
     mustStop("ICAP service connection externally closed");

diff --git a/src/client_side.cc b/src/client_side.cc
index 4aed1a53e00d197d4e615bdd40ef38fdaa829b60..bf2a0bfb285907097c0f8423d11dc34c2a3ad2ac 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -502,6 +502,10 @@ httpRequestFree(void *data)
 /* This is a handler normally called by comm_close() */
 void ConnStateData::connStateClosed(const CommCloseCbParams &)
 {
+    if (clientConnection) {
+        clientConnection->noteClosure();
+        // keep closed clientConnection for logging, clientdb cleanup, etc.
+    }
     deleteThis("ConnStateData::connStateClosed");
 }
 

diff --git a/src/clients/FtpClient.cc b/src/clients/FtpClient.cc
index 9830af8cb9d41ae4b5d609d47ad2e64b634b275f..c277e73fe53d3360fbfbd601086631242f5bde68 100644 (file)
--- a/src/clients/FtpClient.cc
+++ b/src/clients/FtpClient.cc
@@ -790,10 +790,11 @@ void
 Ftp::Client::dataClosed(const CommCloseCbParams &)
 {
     debugs(9, 4, status());
+    if (data.conn)
+        data.conn->noteClosure();
     if (data.listenConn != NULL) {
         data.listenConn->close();
         data.listenConn = NULL;
-        // NP clear() does the: data.fd = -1;
     }
     data.clear();
 }
@@ -858,6 +859,8 @@ void
 Ftp::Client::ctrlClosed(const CommCloseCbParams &)
 {
     debugs(9, 4, status());
+    if (ctrl.conn)
+        ctrl.conn->noteClosure();
     ctrl.clear();
     doneWithFwd = "ctrlClosed()"; // assume FwdState is monitoring too
     mustStop("Ftp::Client::ctrlClosed");

diff --git a/src/clients/HttpTunneler.cc b/src/clients/HttpTunneler.cc
index 3c9998700043da0689ea692bc749856645d08aa9..dd8a4ba947b59a06612d9b466880e327a9302523 100644 (file)
--- a/src/clients/HttpTunneler.cc
+++ b/src/clients/HttpTunneler.cc
@@ -100,6 +100,11 @@ Http::Tunneler::start()
 void
 Http::Tunneler::handleConnectionClosure(const CommCloseCbParams &params)
 {
+    if (connection) {
+        connection->noteClosure();
+        // TODO: Properly get rid of connection here instead of keeping a closed
+        // connection object for peerConnectFailed(),noteUses() in bailWith().
+    }
     closer = nullptr;
     bailWith(new ErrorState(ERR_CONNECT_FAIL, Http::scBadGateway, request.getRaw(), al));
 }
@@ -366,10 +371,15 @@ Http::Tunneler::bailWith(ErrorState *error)
     callBack();
     disconnect();
 
-    if (noteFwdPconnUse)
-        fwdPconnPool->noteUses(fd_table[connection->fd].pconn.uses);
-    // TODO: Reuse to-peer connections after a CONNECT error response.
-    connection->close();
+    // TODO: Close before callBack(); do not pretend to send an open connection.
+    if (Comm::IsConnOpen(connection)) {
+        if (noteFwdPconnUse)
+            fwdPconnPool->noteUses(fd_table[connection->fd].pconn.uses);
+        // TODO: Reuse to-peer connections after a CONNECT error response.
+
+        assert(!closer); // the above disconnect() removes it
+        connection->close();
+    }
     connection = nullptr;
 }
 
@@ -390,12 +400,13 @@ Http::Tunneler::disconnect()
     }
 
     if (reader) {
-        Comm::ReadCancel(connection->fd, reader);
+        if (Comm::IsConnOpen(connection))
+            Comm::ReadCancel(connection->fd, reader);
         reader = nullptr;
     }
 
-    // remove connection timeout handler
-    commUnsetConnTimeout(connection);
+    if (Comm::IsConnOpen(connection))
+        commUnsetConnTimeout(connection);
 }
 
 void
@@ -415,7 +426,7 @@ Http::Tunneler::swanSong()
     AsyncJob::swanSong();
 
     if (callback) {
-        if (requestWritten && tunnelEstablished) {
+        if (requestWritten && tunnelEstablished && Comm::IsConnOpen(connection)) {
             sendSuccess();
         } else {
             // job-ending emergencies like handleStopRequest() or callException()

diff --git a/src/comm.cc b/src/comm.cc
index e49268ddae28fd62fe376b8b02110832031c67a3..89aa25ab439b0eab1b90fe693b0b22e3605bb7a7 100644 (file)
--- a/src/comm.cc
+++ b/src/comm.cc
@@ -743,6 +743,10 @@ commCallCloseHandlers(int fd)
         // If call is not canceled schedule it for execution else ignore it
         if (!call->canceled()) {
             debugs(5, 5, "commCallCloseHandlers: ch->handler=" << call);
+            // XXX: Without the following code, callback fd may be -1.
+            // typedef CommCloseCbParams Params;
+            // auto &params = GetCommParams<Params>(call);
+            // params.fd = fd;
             ScheduleCallHere(call);
         }
     }
@@ -1787,6 +1791,10 @@ DeferredReadManager::CloseHandler(const CommCloseCbParams &params)
     CbDataList<DeferredRead> *temp = (CbDataList<DeferredRead> *)params.data;
 
     temp->element.closer = NULL;
+    if (temp->element.theRead.conn) {
+        temp->element.theRead.conn->noteClosure();
+        temp->element.theRead.conn = nullptr;
+    }
     temp->element.markCancelled();
 }
 
@@ -1860,6 +1868,11 @@ DeferredReadManager::kickARead(DeferredRead const &aRead)
     if (aRead.cancelled)
         return;
 
+    // TODO: This check still allows theReader call with a closed theRead.conn.
+    // If a delayRead() caller has a close connection handler, then such a call
+    // would be useless and dangerous. If a delayRead() caller does not have it,
+    // then the caller will get stuck when an external connection closure makes
+    // aRead.cancelled (checked above) true.
     if (Comm::IsConnOpen(aRead.theRead.conn) && fd_table[aRead.theRead.conn->fd].closing())
         return;
 

diff --git a/src/comm/TcpAcceptor.cc b/src/comm/TcpAcceptor.cc
index 371d6cb06cd09e28adc7e436adbf9b9b9f5c2732..eb35c97fd46134d5d220c42634252e3380399531 100644 (file)
--- a/src/comm/TcpAcceptor.cc
+++ b/src/comm/TcpAcceptor.cc
@@ -206,7 +206,10 @@ void
 Comm::TcpAcceptor::handleClosure(const CommCloseCbParams &)
 {
     closer_ = NULL;
-    conn = NULL;
+    if (conn) {
+        conn->noteClosure();
+        conn = nullptr;
+    }
     Must(done());
 }
 

diff --git a/src/dns_internal.cc b/src/dns_internal.cc
index d18c4bc771e1ebb907d49e753b1e374d6b006b73..ef3355d91a9f9ac0af09b73c1fd1caf497066c3f 100644 (file)
--- a/src/dns_internal.cc
+++ b/src/dns_internal.cc
@@ -872,6 +872,10 @@ static void
 idnsVCClosed(const CommCloseCbParams &params)
 {
     nsvc * vc = (nsvc *)params.data;
+    if (vc->conn) {
+        vc->conn->noteClosure();
+        vc->conn = nullptr;
+    }
     delete vc;
 }
 

diff --git a/src/gopher.cc b/src/gopher.cc
index 6f050394497d17b25372e94985fe08a3135c4b0e..041fff6d183110cc67c88927c52c5287f04359e4 100644 (file)
--- a/src/gopher.cc
+++ b/src/gopher.cc
@@ -145,6 +145,8 @@ static void
 gopherStateFree(const CommCloseCbParams &params)
 {
     GopherStateData *gopherState = (GopherStateData *)params.data;
+    // Assume that FwdState is monitoring and calls noteClosure(). See XXX about
+    // Connection sharing with FwdState in gopherStart().
     delete gopherState;
 }
 
@@ -945,6 +947,7 @@ gopherStart(FwdState * fwd)
         return;
     }
 
+    // XXX: Sharing open Connection with FwdState that has its own handlers/etc.
     gopherState->serverConn = fwd->serverConnection();
     gopherSendRequest(fwd->serverConnection()->fd, gopherState);
     AsyncCall::Pointer timeoutCall = commCbCall(5, 4, "gopherTimeout",

diff --git a/src/ident/Ident.cc b/src/ident/Ident.cc
index ffc2f8526bf2fab2c13e45ef803661b5ea566ed3..3b52f76530ec5e60bc7936771f46a8b3963bbb4d 100644 (file)
--- a/src/ident/Ident.cc
+++ b/src/ident/Ident.cc
@@ -125,7 +125,10 @@ void
 Ident::Close(const CommCloseCbParams &params)
 {
     IdentStateData *state = (IdentStateData *)params.data;
-    // XXX: A Connection closure handler must update its Connection object.
+    if (state->conn) {
+        state->conn->noteClosure();
+        state->conn = nullptr;
+    }
     state->deleteThis("connection closed");
 }
 

diff --git a/src/log/TcpLogger.cc b/src/log/TcpLogger.cc
index c16f6d42a68fb2c0dc85c35e37ec9d323bdf8064..c046689a155f5d757a6223b4a4eb72588d98b855 100644 (file)
--- a/src/log/TcpLogger.cc
+++ b/src/log/TcpLogger.cc
@@ -370,7 +370,10 @@ Log::TcpLogger::handleClosure(const CommCloseCbParams &)
 {
     assert(inCall != NULL);
     closer = NULL;
-    conn = NULL;
+    if (conn) {
+        conn->noteClosure();
+        conn = nullptr;
+    }
     // in all current use cases, we should not try to reconnect
     mustStop("Log::TcpLogger::handleClosure");
 }

diff --git a/src/mgr/Forwarder.cc b/src/mgr/Forwarder.cc
index 0486d01ce6fd5d7aff53a4ceb477eaf2d683046e..fa17cd6e0e48a0bd126d3ef0f37c5c3e304916a9 100644 (file)
--- a/src/mgr/Forwarder.cc
+++ b/src/mgr/Forwarder.cc
@@ -97,7 +97,11 @@ void
 Mgr::Forwarder::noteCommClosed(const CommCloseCbParams &)
 {
     debugs(16, 5, HERE);
-    conn = NULL; // needed?
+    closer = nullptr;
+    if (conn) {
+        conn->noteClosure();
+        conn = nullptr;
+    }
     mustStop("commClosed");
 }
 

diff --git a/src/mgr/Inquirer.cc b/src/mgr/Inquirer.cc
index d38bf3a30b32f7ca7c2c3bb71c5a856259048d53..b6294bcd686d6ab06e5a5f079fe7c085b8defba1 100644 (file)
--- a/src/mgr/Inquirer.cc
+++ b/src/mgr/Inquirer.cc
@@ -110,8 +110,11 @@ void
 Mgr::Inquirer::noteCommClosed(const CommCloseCbParams& params)
 {
     debugs(16, 5, HERE);
-    Must(!Comm::IsConnOpen(conn) && params.conn.getRaw() == conn.getRaw());
-    conn = NULL;
+    closer = nullptr;
+    if (conn) {
+        conn->noteClosure();
+        conn = nullptr;
+    }
     mustStop("commClosed");
 }
 

diff --git a/src/mgr/StoreToCommWriter.cc b/src/mgr/StoreToCommWriter.cc
index 251440eb92d0be0f173580e3c73ef90dd135a82d..7d5bd6e9aaa77f2700dd078d57acee90d649b310 100644 (file)
--- a/src/mgr/StoreToCommWriter.cc
+++ b/src/mgr/StoreToCommWriter.cc
@@ -131,7 +131,11 @@ void
 Mgr::StoreToCommWriter::noteCommClosed(const CommCloseCbParams &)
 {
     debugs(16, 6, HERE);
-    Must(!Comm::IsConnOpen(clientConnection));
+    if (clientConnection) {
+        clientConnection->noteClosure();
+        clientConnection = nullptr;
+    }
+    closer = nullptr;
     mustStop("commClosed");
 }
 

diff --git a/src/security/PeerConnector.cc b/src/security/PeerConnector.cc
index 0f572f9bf0868654e6e59e17013c76dfab71e500..d28169f4df6fa3392104b489c743b8d86cfb97a2 100644 (file)
--- a/src/security/PeerConnector.cc
+++ b/src/security/PeerConnector.cc
@@ -88,9 +88,17 @@ Security::PeerConnector::start()
 void
 Security::PeerConnector::commCloseHandler(const CommCloseCbParams &params)
 {
+    debugs(83, 5, "FD " << params.fd << ", Security::PeerConnector=" << params.data);
+
     closeHandler = nullptr;
+    if (serverConn) {
+        // TODO: Calling PconnPool::noteUses() should not be our responsibility.
+        if (noteFwdPconnUse && serverConn->isOpen())
+            fwdPconnPool->noteUses(fd_table[serverConn->fd].pconn.uses);
+        serverConn->noteClosure();
+        serverConn = nullptr;
+    }
 
-    debugs(83, 5, "FD " << params.fd << ", Security::PeerConnector=" << params.data);
     const auto err = new ErrorState(ERR_SECURE_CONNECT_FAIL, Http::scServiceUnavailable, request.getRaw(), al);
 #if USE_OPENSSL
     err->detail = new Ssl::ErrorDetail(SQUID_ERR_SSL_HANDSHAKE, nullptr, nullptr);
@@ -112,6 +120,8 @@ Security::PeerConnector::commTimeoutHandler(const CommTimeoutCbParams &)
 bool
 Security::PeerConnector::initialize(Security::SessionPointer &serverSession)
 {
+    Must(Comm::IsConnOpen(serverConnection()));
+
     Security::ContextPointer ctx(getTlsContext());
     debugs(83, 5, serverConnection() << ", ctx=" << (void*)ctx.get());
 
@@ -153,6 +163,8 @@ Security::PeerConnector::initialize(Security::SessionPointer &serverSession)
 void
 Security::PeerConnector::recordNegotiationDetails()
 {
+    Must(Comm::IsConnOpen(serverConnection()));
+
     const int fd = serverConnection()->fd;
     Security::SessionPointer session(fd_table[fd].ssl);
 
@@ -171,6 +183,8 @@ Security::PeerConnector::recordNegotiationDetails()
 void
 Security::PeerConnector::negotiate()
 {
+    Must(Comm::IsConnOpen(serverConnection()));
+
     const int fd = serverConnection()->fd;
     if (fd_table[fd].closing())
         return;
@@ -217,6 +231,7 @@ Security::PeerConnector::sslFinalized()
 {
 #if USE_OPENSSL
     if (Ssl::TheConfig.ssl_crt_validator && useCertValidator_) {
+        Must(Comm::IsConnOpen(serverConnection()));
         const int fd = serverConnection()->fd;
         Security::SessionPointer session(fd_table[fd].ssl);
 
@@ -260,6 +275,7 @@ void
 Security::PeerConnector::sslCrtvdHandleReply(Ssl::CertValidationResponse::Pointer validationResponse)
 {
     Must(validationResponse != NULL);
+    Must(Comm::IsConnOpen(serverConnection()));
 
     Ssl::ErrorDetail *errDetails = NULL;
     bool validatorFailed = false;
@@ -308,6 +324,8 @@ Security::PeerConnector::sslCrtvdHandleReply(Ssl::CertValidationResponse::Pointe
 Security::CertErrors *
 Security::PeerConnector::sslCrtvdCheckForErrors(Ssl::CertValidationResponse const &resp, Ssl::ErrorDetail *& errDetails)
 {
+    Must(Comm::IsConnOpen(serverConnection()));
+
     ACLFilledChecklist *check = NULL;
     Security::SessionPointer session(fd_table[serverConnection()->fd].ssl);
 
@@ -383,6 +401,8 @@ Security::PeerConnector::negotiateSsl()
 void
 Security::PeerConnector::handleNegotiateError(const int ret)
 {
+    Must(Comm::IsConnOpen(serverConnection()));
+
     const int fd = serverConnection()->fd;
     const Security::SessionPointer session(fd_table[fd].ssl);
     unsigned long ssl_lib_error = ret;
@@ -447,8 +467,10 @@ Security::PeerConnector::handleNegotiateError(const int ret)
 void
 Security::PeerConnector::noteWantRead()
 {
-    const int fd = serverConnection()->fd;
     debugs(83, 5, serverConnection());
+
+    Must(Comm::IsConnOpen(serverConnection()));
+    const int fd = serverConnection()->fd;
 #if USE_OPENSSL
     Security::SessionPointer session(fd_table[fd].ssl);
     BIO *b = SSL_get_rbio(session.get());
@@ -485,8 +507,10 @@ Security::PeerConnector::noteWantRead()
 void
 Security::PeerConnector::noteWantWrite()
 {
-    const int fd = serverConnection()->fd;
     debugs(83, 5, serverConnection());
+    Must(Comm::IsConnOpen(serverConnection()));
+
+    const int fd = serverConnection()->fd;
     Comm::SetSelect(fd, COMM_SELECT_WRITE, &NegotiateSsl, new Pointer(this), 0);
     return;
 }
@@ -507,6 +531,7 @@ Security::PeerConnector::noteNegotiationError(const int ret, const int ssl_error
 #endif
     int xerr = errno;
 
+    Must(Comm::IsConnOpen(serverConnection()));
     const int fd = serverConnection()->fd;
     debugs(83, DBG_IMPORTANT, "ERROR: negotiating TLS on FD " << fd <<
            ": " << Security::ErrorString(ssl_lib_error) << " (" <<
@@ -547,15 +572,21 @@ Security::PeerConnector::bail(ErrorState *error)
     Must(dialer);
     dialer->answer().error = error;
 
-    if (const auto p = serverConnection()->getPeer())
-        peerConnectFailed(p);
+    if (serverConnection()) {
+        if (const auto p = serverConnection()->getPeer())
+            peerConnectFailed(p);
+    }
 
     callBack();
     disconnect();
 
-    if (noteFwdPconnUse)
-        fwdPconnPool->noteUses(fd_table[serverConn->fd].pconn.uses);
-    serverConn->close();
+    // TODO: Close before callBack(); do not pretend to send an open connection.
+    if (Comm::IsConnOpen(serverConn)) {
+        if (noteFwdPconnUse)
+            fwdPconnPool->noteUses(fd_table[serverConn->fd].pconn.uses);
+        assert(!closeHandler); // the above disconnect() removes it
+        serverConn->close();
+    }
     serverConn = nullptr;
 }
 
@@ -569,6 +600,9 @@ Security::PeerConnector::sendSuccess()
 void
 Security::PeerConnector::disconnect()
 {
+    if (!Comm::IsConnOpen(serverConnection()))
+        return;
+
     if (closeHandler) {
         comm_remove_close_handler(serverConnection()->fd, closeHandler);
         closeHandler = nullptr;
@@ -588,7 +622,7 @@ Security::PeerConnector::callBack()
     callback = NULL; // this should make done() true
     CbDialer *dialer = dynamic_cast<CbDialer*>(cb->getDialer());
     Must(dialer);
-    dialer->answer().conn = serverConnection();
+    dialer->answer().conn = serverConnection(); // may be nil
     ScheduleCallHere(cb);
 }
 
@@ -620,7 +654,7 @@ Security::PeerConnector::status() const
         buf.append("Stopped, reason:", 16);
         buf.appendf("%s",stopReason);
     }
-    if (serverConn != NULL)
+    if (Comm::IsConnOpen(serverConn))
         buf.appendf(" FD %d", serverConn->fd);
     buf.appendf(" %s%u]", id.prefix(), id.value);
     buf.terminate();
@@ -667,6 +701,7 @@ Security::PeerConnector::certDownloadingDone(SBuf &obj, int downloadStatus)
     debugs(81, 5, "Certificate downloading status: " << downloadStatus << " certificate size: " << obj.length());
 
     // get ServerBio from SSL object
+    Must(Comm::IsConnOpen(serverConnection()));
     const int fd = serverConnection()->fd;
     Security::SessionPointer session(fd_table[fd].ssl);
     BIO *b = SSL_get_rbio(session.get());
@@ -714,6 +749,7 @@ Security::PeerConnector::checkForMissingCertificates()
     if (csd && csd->nestedLevel() >= MaxNestedDownloads)
         return false;
 
+    Must(Comm::IsConnOpen(serverConnection()));
     const int fd = serverConnection()->fd;
     Security::SessionPointer session(fd_table[fd].ssl);
     BIO *b = SSL_get_rbio(session.get());

diff --git a/src/snmp/Forwarder.cc b/src/snmp/Forwarder.cc
index 51b2932272e8059871276a988bfdc339e1ed22a0..961818f0ef970506e37a6ded3b6e0fc23b7b116d 100644 (file)
--- a/src/snmp/Forwarder.cc
+++ b/src/snmp/Forwarder.cc
@@ -53,6 +53,7 @@ Snmp::Forwarder::noteCommClosed(const CommCloseCbParams& params)
 {
     debugs(49, 5, HERE);
     Must(fd == params.fd);
+    closer = nullptr;
     fd = -1;
     mustStop("commClosed");
 }
@@ -68,8 +69,7 @@ void
 Snmp::Forwarder::handleException(const std::exception& e)
 {
     debugs(49, 3, HERE << e.what());
-    if (fd >= 0)
-        sendError(SNMP_ERR_GENERR);
+    sendError(SNMP_ERR_GENERR);
     Ipc::Forwarder::handleException(e);
 }
 
@@ -78,6 +78,10 @@ void
 Snmp::Forwarder::sendError(int error)
 {
     debugs(49, 3, HERE);
+
+    if (fd < 0)
+        return; // client gone
+
     Snmp::Request& req = static_cast<Snmp::Request&>(*request);
     req.pdu.command = SNMP_PDU_RESPONSE;
     req.pdu.errstat = error;

diff --git a/src/snmp/Inquirer.cc b/src/snmp/Inquirer.cc
index f69388ed39b6b60c701e0af870a1d534b8ed79e0..589e527c0a7f187de489bc0f901895da6b80db33 100644 (file)
--- a/src/snmp/Inquirer.cc
+++ b/src/snmp/Inquirer.cc
@@ -88,7 +88,11 @@ Snmp::Inquirer::noteCommClosed(const CommCloseCbParams& params)
 {
     debugs(49, 5, HERE);
     Must(!Comm::IsConnOpen(conn) || conn->fd == params.conn->fd);
-    conn = NULL;
+    closer = nullptr;
+    if (conn) {
+        conn->noteClosure();
+        conn = nullptr;
+    }
     mustStop("commClosed");
 }
 
@@ -102,6 +106,10 @@ void
 Snmp::Inquirer::sendResponse()
 {
     debugs(49, 5, HERE);
+
+    if (!Comm::IsConnOpen(conn))
+        return; // client gone
+
     aggrPdu.fixAggregate();
     aggrPdu.command = SNMP_PDU_RESPONSE;
     u_char buffer[SNMP_REQUEST_SIZE];

diff --git a/src/whois.cc b/src/whois.cc
index a1a3beb7b1eb59961e733c4a60ccdbfea20db432..fddb338945c0ad90a8251061d0ce30672acd938c 100644 (file)
--- a/src/whois.cc
+++ b/src/whois.cc
@@ -169,6 +169,7 @@ whoisClose(const CommCloseCbParams &params)
 {
     WhoisState *p = (WhoisState *)params.data;
     debugs(75, 3, "whoisClose: FD " << params.fd);
+    // We do not own a Connection. Assume that FwdState is also monitoring.
     p->entry->unlock("whoisClose");
     delete p;
 }