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 e21b69723634e9e6d723d0cbd28fa3edd36e69cc..a4e860dc36cead8c81de17a5aac4d10df9a86028 100644 (file)
--- a/src/FwdState.cc
+++ b/src/FwdState.cc
@@ -123,7 +123,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();
 }
@@ -456,7 +457,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;
 }
 
@@ -687,7 +689,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) {
@@ -865,7 +867,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);
 
@@ -904,7 +907,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->url.host());
 

diff --git a/src/FwdState.h b/src/FwdState.h
index e38cbf09d3aeeda3464eb0ef0b5e3d396d51a9d1..a069c2e934eea61b88b19977dc0e99c2ca18ee4d 100644 (file)
--- a/src/FwdState.h
+++ b/src/FwdState.h
@@ -153,6 +153,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 086b612bc5008fd2c7a5a3a630d9a4e6cf46a971..68d1fdac66b573c5eba338a3cb9b4a25b826b6d0 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 e7b62176e1093f5727ddd48778a30a9ab74e4221..476d8ced42fbe5804cc14b39bea74e82b01b497b 100644 (file)
--- a/src/comm.cc
+++ b/src/comm.cc
@@ -972,7 +972,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=" <<
@@ -981,6 +981,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 8bc14a3f46e648ca4409246bd962e484871c094a..af712ed82a5a960290cde47b84ba362780b310b3 100644 (file)
--- a/src/http.cc
+++ b/src/http.cc
@@ -166,7 +166,8 @@ HttpStateData::httpTimeout(const CommTimeoutCbParams &)
         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
@@ -1234,8 +1235,8 @@ HttpStateData::readReply(const CommIoCbParams &io)
         err->xerrno = rd.xerrno;
         fwd->fail(err);
         flags.do_next_read = false;
-        io.conn->close();
-
+        closeServer();
+        mustStop("HttpStateData::readReply");
         return;
     }
 
@@ -1335,7 +1336,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
 }
 
@@ -1599,7 +1601,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;
     }
 
@@ -1627,7 +1630,6 @@ HttpStateData::sendComplete()
     request->hier.peer_http_request_sent = current_time;
 }
 
-// Close the HTTP server connection. Used by serverComplete().
 void
 HttpStateData::closeServer()
 {
@@ -2426,7 +2428,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;
             }
         }