From: Christos Tsantilas Date: Fri, 9 Dec 2016 01:58:33 +0000 (+1300) Subject: Bug 4174 partial: fix Write.cc:41 "!ccb->active()" assertion. X-Git-Tag: SQUID_3_5_23~9 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=c091059b1ad1cc03def5a6402ceb8449d2068f6b;p=thirdparty%2Fsquid.git Bug 4174 partial: fix Write.cc:41 "!ccb->active()" assertion. The following sequence of events triggers this assertion: - The server sends an 1xx control message. - http.cc schedules ConnStateData::sendControlMsg call. - Before sendControlMsg is fired, http.cc detects an error (e.g., I/O error or timeout) and starts writing the reply to the user. - The ConnStateData::sendControlMsg is fired, starts writing 1xx, and hits the "no concurrent writes" assertion. We could only reproduce this sequence in the lab after changing Squid code to trigger a timeout at the right moment, but the sequence looks plausible. Other event sequences might result in the same outcome. To avoid concurrent writes, Squid now drops the control message if Http::One::Server detects that a reply is already being written. Also, ConnStateData delays reply writing until a pending control message write has been completed. This is a Measurement Factory project. --- diff --git a/src/client_side.cc b/src/client_side.cc index afadd8014a..e744151d3c 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -340,7 +340,21 @@ ClientSocketContext::writeControlMsg(HttpControlMsg &msg) AsyncCall::Pointer call = commCbCall(33, 5, "ClientSocketContext::wroteControlMsg", CommIoCbPtrFun(&WroteControlMsg, this)); - getConn()->writeControlMsgAndCall(this, rep.getRaw(), call); + if (!getConn()->writeControlMsgAndCall(this, rep.getRaw(), call)) { + // but still inform the caller (so it may resume its operation) + doneWithControlMsg(); + } +} + +void +ClientSocketContext::doneWithControlMsg() +{ + ScheduleCallHere(cbControlMsgSent); + cbControlMsgSent = NULL; + + debugs(33, 3, clientConnection << ": calling PushDeferredIfNeeded after control msg wrote"); + ClientSocketContextPushDeferredIfNeeded(this, getConn()); + } /// called when we wrote the 1xx response @@ -351,7 +365,7 @@ ClientSocketContext::wroteControlMsg(const Comm::ConnectionPointer &conn, char * return; if (errflag == Comm::OK) { - ScheduleCallHere(cbControlMsgSent); + doneWithControlMsg(); return; } @@ -1455,6 +1469,8 @@ clientSocketRecipient(clientStreamNode * node, ClientHttpRequest * http, if (context != http->getConn()->getCurrentContext()) context->deferRecipientForLater(node, rep, receivedData); + else if (context->controlMsgIsPending()) + context->deferRecipientForLater(node, rep, receivedData); else http->getConn()->handleReply(rep, receivedData); diff --git a/src/client_side.h b/src/client_side.h index e489e6946f..9a772ae4cf 100644 --- a/src/client_side.h +++ b/src/client_side.h @@ -129,9 +129,13 @@ public: /// starts writing 1xx control message to the client void writeControlMsg(HttpControlMsg &msg); + /// true if 1xx to the user is pending + bool controlMsgIsPending() {return cbControlMsgSent != NULL;} + protected: static IOCB WroteControlMsg; void wroteControlMsg(const Comm::ConnectionPointer &conn, char *bufnotused, size_t size, Comm::Flag errflag, int xerrno); + void doneWithControlMsg(); private: void prepareReply(HttpReply * rep); @@ -387,7 +391,7 @@ public: void connectionTag(const char *aTag) { connectionTag_ = aTag; } /// handle a control message received by context from a peer and call back - virtual void writeControlMsgAndCall(ClientSocketContext *context, HttpReply *rep, AsyncCall::Pointer &call) = 0; + virtual bool writeControlMsgAndCall(ClientSocketContext *context, HttpReply *rep, AsyncCall::Pointer &call) = 0; /// ClientStream calls this to supply response header (once) and data /// for the current ClientSocketContext. diff --git a/src/servers/FtpServer.cc b/src/servers/FtpServer.cc index 952fbd07eb..ea3dab76b3 100644 --- a/src/servers/FtpServer.cc +++ b/src/servers/FtpServer.cc @@ -1152,12 +1152,13 @@ Ftp::Server::writeForwardedForeign(const HttpReply *reply) writeErrorReply(reply, 451); } -void +bool Ftp::Server::writeControlMsgAndCall(ClientSocketContext *context, HttpReply *reply, AsyncCall::Pointer &call) { // the caller guarantees that we are dealing with the current context only // the caller should also make sure reply->header.has(HDR_FTP_STATUS) writeForwardedReplyAndCall(reply, call); + return true; } void diff --git a/src/servers/FtpServer.h b/src/servers/FtpServer.h index 76107060e8..93fe6b8a73 100644 --- a/src/servers/FtpServer.h +++ b/src/servers/FtpServer.h @@ -94,7 +94,7 @@ protected: virtual void clientPinnedConnectionClosed(const CommCloseCbParams &io); virtual void handleReply(HttpReply *header, StoreIOBuffer receivedData); virtual int pipelinePrefetchMax() const; - virtual void writeControlMsgAndCall(ClientSocketContext *context, HttpReply *rep, AsyncCall::Pointer &call); + virtual bool writeControlMsgAndCall(ClientSocketContext *context, HttpReply *rep, AsyncCall::Pointer &call); virtual time_t idleTimeout() const; /* BodyPipe API */ diff --git a/src/servers/HttpServer.cc b/src/servers/HttpServer.cc index 7d854239de..59e8967c9a 100644 --- a/src/servers/HttpServer.cc +++ b/src/servers/HttpServer.cc @@ -35,7 +35,7 @@ protected: virtual ClientSocketContext *parseOneRequest(Http::ProtocolVersion &ver); virtual void processParsedRequest(ClientSocketContext *context, const Http::ProtocolVersion &ver); virtual void handleReply(HttpReply *rep, StoreIOBuffer receivedData); - virtual void writeControlMsgAndCall(ClientSocketContext *context, HttpReply *rep, AsyncCall::Pointer &call); + virtual bool writeControlMsgAndCall(ClientSocketContext *context, HttpReply *rep, AsyncCall::Pointer &call); virtual time_t idleTimeout() const; /* BodyPipe API */ @@ -167,9 +167,16 @@ Http::Server::handleReply(HttpReply *rep, StoreIOBuffer receivedData) context->sendStartOfMessage(rep, receivedData); } -void +bool Http::Server::writeControlMsgAndCall(ClientSocketContext *context, HttpReply *rep, AsyncCall::Pointer &call) { + // Ignore this late control message if we have started sending a + // reply to the user already (e.g., after an error). + if (context->reply) { + debugs(11, 2, "drop 1xx made late by " << context->reply); + return false; + } + // apply selected clientReplyContext::buildReplyHeader() mods // it is not clear what headers are required for control messages rep->header.removeHopByHopEntries(); @@ -184,6 +191,7 @@ Http::Server::writeControlMsgAndCall(ClientSocketContext *context, HttpReply *re Comm::Write(context->clientConnection, mb, call); delete mb; + return true; } ConnStateData *