Author: Alex Rousskov <rousskov@measurement-factory.com>

Bug 2896 fix: assertion failed: comm.cc:2063: "!fd_table[fd].closing()"

When comm_close() has been called for the server fd but the close handler has
not yet been activated, the Server may receive an async call not associated
with the fd (e.g., more request body data coming from the HTTP or ICAP client)
that prompts the server to write to the fd.  We now check whether it is still
safe to write before writing. If it is not safe, we do not write but wait for
our close handler to be called.

TODO: when all comm_write callers check for fd closing, comm API can be
redefined to drop unsafe calls instead of asserting.

diff --git a/src/Server.cc b/src/Server.cc
index 5f00e522952a3a384f55ef15ef012073a7755248..52e0f94203f0b49f4f7f67d101bb4acc80da7105 100644 (file)
--- a/src/Server.cc
+++ b/src/Server.cc
@@ -35,6 +35,7 @@
 #include "squid.h"
 #include "Server.h"
 #include "Store.h"
+#include "fde.h" /* for fd_table[fd].closing */
 #include "HttpRequest.h"
 #include "HttpReply.h"
 #include "TextException.h"
@@ -393,18 +394,32 @@ ServerStateData::sentRequestBody(const CommIoCbParams &io)
         sendMoreRequestBody();
 }
 
+bool
+ServerStateData::canSend(int fd) const
+{
+    return fd >= 0 && !fd_table[fd].closing();
+}
+
 void
 ServerStateData::sendMoreRequestBody()
 {
     assert(requestBodySource != NULL);
     assert(!requestSender);
+
+    const int fd = dataDescriptor();
+
+    if (!canSend(fd)) {
+        debugs(9,3, HERE << "cannot send request body to closing FD " << fd);
+        return; // wait for the kid's close handler; TODO: assert(closer);
+    }
+
     MemBuf buf;
     if (requestBodySource->getMoreData(buf)) {
         debugs(9,3, HERE << "will write " << buf.contentSize() << " request body bytes");
         typedef CommCbMemFunT<ServerStateData, CommIoCbParams> Dialer;
         requestSender = asyncCall(93,3, "ServerStateData::sentRequestBody",
                                   Dialer(this, &ServerStateData::sentRequestBody));
-        comm_write_mbuf(dataDescriptor(), &buf, requestSender);
+        comm_write_mbuf(fd, &buf, requestSender);
     } else {
         debugs(9,3, HERE << "will wait for more request body bytes or eof");
         requestSender = NULL;

diff --git a/src/Server.h b/src/Server.h
index cf6516ca1439918711dd1497dafbcfc8ee8615a1..27e722ab9d73accd47242dfde7e430406ad19b96 100644 (file)
--- a/src/Server.h
+++ b/src/Server.h
@@ -128,6 +128,8 @@ protected:
     void handleRequestBodyProductionEnded();
     virtual void handleRequestBodyProducerAborted() = 0;
 
+    /// whether it is not too late to write to the server
+    bool canSend(int fd) const;
     // sending of the request body to the server
     void sendMoreRequestBody();
     // has body; kids overwrite to increment I/O stats counters

diff --git a/src/ftp.cc b/src/ftp.cc
index 171a6da0b340f4e2101336bdd2afff7e56d168ed..78d869470de522ed29d1d82a5860671468f894a7 100644 (file)
--- a/src/ftp.cc
+++ b/src/ftp.cc
@@ -1670,6 +1670,12 @@ FtpStateData::writeCommand(const char *buf)
 
     ctrl.last_command = ebuf;
 
+    if (!canSend(ctrl.fd)) {
+        debugs(9, 2, HERE << "cannot send to closing ctrl FD " << ctrl.fd);
+        // TODO: assert(ctrl.closer != NULL);
+        return;
+    }
+
     typedef CommCbMemFunT<FtpStateData, CommIoCbParams> Dialer;
     AsyncCall::Pointer call = asyncCall(9, 5, "FtpStateData::ftpWriteCommandCallback",
                                         Dialer(this, &FtpStateData::ftpWriteCommandCallback));

diff --git a/src/http.cc b/src/http.cc
index bcf2db9f008ef5c52183113f6b8704402a78f710..c714e6f0bc26de3b7d94c1f80ccbeed727717afe 100644 (file)
--- a/src/http.cc
+++ b/src/http.cc
@@ -1979,6 +1979,13 @@ HttpStateData::sendRequest()
     MemBuf mb;
 
     debugs(11, 5, "httpSendRequest: FD " << fd << ", request " << request << ", this " << this << ".");
+
+    if (!canSend(fd)) {
+        debugs(11,3, HERE << "cannot send request to closing FD " << fd);
+        assert(closeHandler != NULL);
+        return false;
+    }
+
     typedef CommCbMemFunT<HttpStateData, CommTimeoutCbParams> TimeoutDialer;
     AsyncCall::Pointer timeoutCall =  asyncCall(11, 5, "HttpStateData::httpTimeout",
                                       TimeoutDialer(this,&HttpStateData::httpTimeout));
@@ -2081,6 +2088,13 @@ HttpStateData::doneSendingRequestBody()
             sendComplete(io);
         } else {
             debugs(11, 2, "doneSendingRequestBody: matched brokenPosts");
+
+            if (!canSend(fd)) {
+                debugs(11,2, HERE << "cannot send CRLF to closing FD " << fd);
+                assert(closeHandler != NULL);
+                return;
+            }
+
             typedef CommCbMemFunT<HttpStateData, CommIoCbParams> Dialer;
             Dialer dialer(this, &HttpStateData::sendComplete);
             AsyncCall::Pointer call= asyncCall(11,5, "HttpStateData::SendComplete", dialer);