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 1b7c820d2338f942a618bfe28c5b1dd40ef37390..6782881022695d4ebd849d79f945740705e42814 100644 (file)
--- a/src/Server.cc
+++ b/src/Server.cc
@@ -36,6 +36,7 @@
 #include "base/TextException.h"
 #include "Server.h"
 #include "Store.h"
+#include "fde.h" /* for fd_table[fd].closing */
 #include "HttpRequest.h"
 #include "HttpReply.h"
 #include "errorpage.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 eff4ddc7f6b7562183fa3c7d5b1e21791044951a..4cf5be59008405d78c7ba9c3c33e965e99df8cb9 100644 (file)
--- a/src/ftp.cc
+++ b/src/ftp.cc
@@ -1521,6 +1521,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 d10bea993f22a8e731073b989f4f477db6cd74da..b5fd423df650d03722daf1b8863a6b254fd4c1df 100644 (file)
--- a/src/http.cc
+++ b/src/http.cc
@@ -1992,6 +1992,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));
@@ -2095,6 +2102,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);