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 afadd8014a9b1005fad12fbe5641465c05eb8c06..e744151d3c0f8883e581b9cc65dd0436cad23823 100644 (file)
--- 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 e489e6946f382a24c587b71c5d4fe3e1a8b65514..9a772ae4cf76496d4ef74b9778aa09b2a88093e1 100644 (file)
--- 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 952fbd07ebf4a8a12dfc3f3a99ea1aed4122f1f8..ea3dab76b3399d4ad50d0bb5e4ba7a6f6a6d5e94 100644 (file)
--- 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 76107060e8a455e72e8f9a58ed8738e817cc833b..93fe6b8a735866453e5e80599a4ff0c77387f950 100644 (file)
--- 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 7d854239de6013917a0f141de79a4c14e4d75caa..59e8967c9ac552de88e93d4b921023e3cda0b8c3 100644 (file)
--- 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 *