From: Christos Tsantilas Date: Mon, 29 Feb 2016 18:43:03 +0000 (+0200) Subject: Bug 4447:FwdState.cc:447 "serverConnection() == conn" assertion X-Git-Tag: SQUID_4_0_8~54 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=398bc06602eb8e7ab32170d6216aa6ddd32e33c4;p=thirdparty%2Fsquid.git 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 e21b697236..a4e860dc36 100644 --- 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 e38cbf09d3..a069c2e934 100644 --- 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 086b612bc5..68d1fdac66 100644 --- 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 e7b62176e1..476d8ced42 100644 --- 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 0f21cf2f6c..9ccd68db77 100644 --- 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 8bc14a3f46..af712ed82a 100644 --- 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; } }