Bug 4447:FwdState.cc:447 "serverConnection() == conn" assertion

After certain failures, FwdState::retryOrBail() may be called twice,
once from FwdState::unregisterdServerEnd() [called from
HttpStateData::swanSong()] and once from the FwdState's own connection
close handler. This may result in two concurrent connections to the
remote server, followed by an assertion upon a connection closure.

This patch:

 - After HttpStateData failures, instead of closing the squid-to-peer
   connection directly (and, hence, triggering closure handlers), calls
   HttpStateData::closeServer() and mustStop() for a cleaner exit with
   fewer wasteful side effects and better debugging.

 - Creates and remembers a FwdState close handler AsyncCall so that
   comm_remove_close_handler() can cancel an already scheduled callback.
   The conversion to the AsyncCall was necessary because legacy [close
   handler callbacks] cannot be canceled once scheduled.

This is a Measurement Factory project.

diff --git a/src/FwdState.cc b/src/FwdState.cc
index 97b1277573f2c0ae200bdbf5a76b09f3bb4cbf59..f16acd0042126a376fa3c53ac3125f6869bd6d71 100644 (file)
--- a/src/FwdState.cc
+++ b/src/FwdState.cc
@@ -119,7 +119,8 @@ void
 FwdState::closeServerConnection(const char *reason)
 {
     debugs(17, 3, "because " << reason << "; " << serverConn);
-    comm_remove_close_handler(serverConn->fd, fwdServerClosedWrapper, this);
+    comm_remove_close_handler(serverConn->fd, closeHandler);
+    closeHandler = NULL;
     fwdPconnPool->noteUses(fd_table[serverConn->fd].pconn.uses);
     serverConn->close();
 }
@@ -446,7 +447,8 @@ FwdState::unregister(Comm::ConnectionPointer &conn)
     debugs(17, 3, HERE << entry->url() );
     assert(serverConnection() == conn);
     assert(Comm::IsConnOpen(conn));
-    comm_remove_close_handler(conn->fd, fwdServerClosedWrapper, this);
+    comm_remove_close_handler(conn->fd, closeHandler);
+    closeHandler = NULL;
     serverConn = NULL;
 }
 
@@ -677,7 +679,7 @@ FwdState::connectDone(const Comm::ConnectionPointer &conn, Comm::Flag status, in
     serverConn = conn;
     debugs(17, 3, HERE << serverConnection() << ": '" << entry->url() << "'" );
 
-    comm_add_close_handler(serverConnection()->fd, fwdServerClosedWrapper, this);
+    closeHandler = comm_add_close_handler(serverConnection()->fd, fwdServerClosedWrapper, this);
 
 #if USE_OPENSSL
     if (!request->flags.pinned) {
@@ -833,7 +835,8 @@ FwdState::connectStart()
             request->flags.pinned = true;
             if (pinned_connection->pinnedAuth())
                 request->flags.auth = true;
-            comm_add_close_handler(serverConn->fd, fwdServerClosedWrapper, this);
+
+            closeHandler = comm_add_close_handler(serverConn->fd,  fwdServerClosedWrapper, this);
 
             syncWithServerConn(pinned_connection->pinning.host);
 
@@ -872,7 +875,7 @@ FwdState::connectStart()
         debugs(17, 3, HERE << "reusing pconn " << serverConnection());
         ++n_tries;
 
-        comm_add_close_handler(serverConnection()->fd, fwdServerClosedWrapper, this);
+        closeHandler = comm_add_close_handler(serverConnection()->fd,  fwdServerClosedWrapper, this);
 
         syncWithServerConn(request->GetHost());
 

diff --git a/src/FwdState.h b/src/FwdState.h
index 8ab0c4d921ac03214eaf8c0f6f272992587978fa..b5220391917339bfef06ca7990e215ed80269134 100644 (file)
--- a/src/FwdState.h
+++ b/src/FwdState.h
@@ -152,6 +152,8 @@ private:
 
     Comm::ConnectionPointer serverConn; ///< a successfully opened connection to a server.
 
+    AsyncCall::Pointer closeHandler; ///< The serverConn close handler
+
     /// possible pconn race states
     typedef enum { raceImpossible, racePossible, raceHappened } PconnRace;
     PconnRace pconnRace; ///< current pconn race state

diff --git a/src/clients/Client.h b/src/clients/Client.h
index 8181cd10079a6ce69bd3602b4b6cecd84c2bfc64..6fc095a89b5e251f561772f336d630a3df9c4eb6 100644 (file)
--- a/src/clients/Client.h
+++ b/src/clients/Client.h
@@ -104,7 +104,9 @@ protected:
     virtual void sentRequestBody(const CommIoCbParams &io) = 0;
     virtual void doneSendingRequestBody() = 0;
 
-    virtual void closeServer() = 0;            /**< end communication with the server */
+    /// Use this to end communication with the server. The call cancels our
+    /// closure handler and tells FwdState to forget about the connection.
+    virtual void closeServer() = 0;
     virtual bool doneWithServer() const = 0;   /**< did we end communication? */
     /// whether we may receive more virgin response body bytes
     virtual bool mayReadVirginReplyBody() const = 0;

diff --git a/src/comm.cc b/src/comm.cc
index 25bdb55b3129cd0786b0400394205e262dde62b1..493d81d23e1c87fcb861706cbfcfbe177cd9ec48 100644 (file)
--- a/src/comm.cc
+++ b/src/comm.cc
@@ -976,7 +976,7 @@ comm_udp_sendto(int fd,
     return Comm::COMM_ERROR;
 }
 
-void
+AsyncCall::Pointer
 comm_add_close_handler(int fd, CLCB * handler, void *data)
 {
     debugs(5, 5, "comm_add_close_handler: FD " << fd << ", handler=" <<
@@ -985,6 +985,7 @@ comm_add_close_handler(int fd, CLCB * handler, void *data)
     AsyncCall::Pointer call=commCbCall(5,4, "SomeCloseHandler",
                                        CommCloseCbPtrFun(handler, data));
     comm_add_close_handler(fd, call);
+    return call;
 }
 
 void

diff --git a/src/comm.h b/src/comm.h
index 0f21cf2f6cdb5ea41c0ea73d5725fc574eefc8a5..9ccd68db77ecf7723375d966e672ce312475620e 100644 (file)
--- a/src/comm.h
+++ b/src/comm.h
@@ -79,7 +79,7 @@ int ignoreErrno(int);
 void commCloseAllSockets(void);
 void checkTimeouts(void);
 
-void comm_add_close_handler(int fd, CLCB *, void *);
+AsyncCall::Pointer comm_add_close_handler(int fd, CLCB *, void *);
 void comm_add_close_handler(int fd, AsyncCall::Pointer &);
 void comm_remove_close_handler(int fd, CLCB *, void *);
 void comm_remove_close_handler(int fd, AsyncCall::Pointer &);

diff --git a/src/http.cc b/src/http.cc
index eb148824e00308c08cdbd9033564c45994602e0d..99c683843a0364a81d7eef3f84ade97b26f6d9d0 100644 (file)
--- a/src/http.cc
+++ b/src/http.cc
@@ -165,7 +165,8 @@ HttpStateData::httpTimeout(const CommTimeoutCbParams &params)
         fwd->fail(new ErrorState(ERR_READ_TIMEOUT, Http::scGatewayTimeout, fwd->request));
     }
 
-    serverConnection->close();
+    closeServer();
+    mustStop("HttpStateData::httpTimeout");
 }
 
 /// Remove an existing public store entry if the incoming response (to be
@@ -1150,7 +1151,8 @@ HttpStateData::readReply(const CommIoCbParams &io)
             err->xerrno = io.xerrno;
             fwd->fail(err);
             flags.do_next_read = false;
-            serverConnection->close();
+            closeServer();
+            mustStop("HttpStateData::readReply");
         }
 
         return;
@@ -1306,7 +1308,8 @@ HttpStateData::continueAfterParsingHeader()
     entry->reset();
     fwd->fail(new ErrorState(error, Http::scBadGateway, fwd->request));
     flags.do_next_read = false;
-    serverConnection->close();
+    closeServer();
+    mustStop("HttpStateData::continueAfterParsingHeader");
     return false; // quit on error
 }
 
@@ -1540,7 +1543,8 @@ HttpStateData::wroteLast(const CommIoCbParams &io)
         ErrorState *err = new ErrorState(ERR_WRITE_ERROR, Http::scBadGateway, fwd->request);
         err->xerrno = io.xerrno;
         fwd->fail(err);
-        serverConnection->close();
+        closeServer();
+        mustStop("HttpStateData::wroteLast");
         return;
     }
 
@@ -1568,7 +1572,6 @@ HttpStateData::sendComplete()
     request->hier.peer_http_request_sent = current_time;
 }
 
-// Close the HTTP server connection. Used by serverComplete().
 void
 HttpStateData::closeServer()
 {
@@ -2371,7 +2374,8 @@ HttpStateData::handleMoreRequestBodyAvailable()
             debugs(11, DBG_IMPORTANT, "http handleMoreRequestBodyAvailable: Likely proxy abuse detected '" << request->client_addr << "' -> '" << entry->url() << "'" );
 
             if (virginReply()->sline.status() == Http::scInvalidHeader) {
-                serverConnection->close();
+                closeServer();
+                mustStop("HttpStateData::handleMoreRequestBodyAvailable");
                 return;
             }
         }