Author: Stefan Fritsch <sf@sfritsch.de>

Bug 3096: Squid destroys CbDataList<DeferredRead> objects too late

When server download speed exceeds client download speed, Squid creates a
CbDataList<DeferredRead> object and associates a comm_close handler with it.
When the server kicks the deferred read, the comm_close handler is canceled.
This create/cancel sequence happens every time the server-side code wants to
read but has to wait for the client, which may happen hundreds of times per
second.

Before this change, those canceled comm_close handlers were not removed from
Comm until the end of the entire server transaction, possibly accumulating
thousands of CbDataList<DeferredRead> objects tied to the socket descriptor
via the canceled but still stored close handler.

comm_remove_close_handler now immediately removes canceled close handlers to
avoid their accumulation.

diff --git a/src/base/AsyncCall.cc b/src/base/AsyncCall.cc
index 9bca17efe87bc72c149fd1fedda0d2c14ceaf472..a5eb37df7eca50ba9008935a05193fc825d66157 100644 (file)
--- a/src/base/AsyncCall.cc
+++ b/src/base/AsyncCall.cc
@@ -69,6 +69,16 @@ AsyncCall::print(std::ostream &os)
         os << "(?" << this << "?)";
 }
 
+void
+AsyncCall::dequeue(AsyncCall::Pointer &head, AsyncCall::Pointer &prev)
+{
+    if (prev != NULL)
+        prev->setNext(Next());
+    else
+        head = Next();
+    setNext(NULL);
+}
+
 bool
 ScheduleCall(const char *fileName, int fileLine, AsyncCall::Pointer &call)
 {

diff --git a/src/base/AsyncCall.h b/src/base/AsyncCall.h
index d4df9f64414ed5589fbf6479abeafa0d7b20923e..29c7f197870ca96e1d7f4461a883b9860d15e4ab 100644 (file)
--- a/src/base/AsyncCall.h
+++ b/src/base/AsyncCall.h
@@ -57,6 +57,9 @@ public:
 
     void print(std::ostream &os);
 
+    /// remove us from the queue; we are head unless we are queued after prev
+    void dequeue(AsyncCall::Pointer &head, AsyncCall::Pointer &prev);
+
     void setNext(AsyncCall::Pointer aNext) {
         theNext = aNext;
     }

diff --git a/src/comm.cc b/src/comm.cc
index b5e927f5f39a86460b611f5bf59a95ac145efa1b..c74425e4a6dfe93f97f0d0203e4b412c20add16d 100644 (file)
--- a/src/comm.cc
+++ b/src/comm.cc
@@ -1721,8 +1721,8 @@ comm_remove_close_handler(int fd, PF * handler, void *data)
     debugs(5, 5, "comm_remove_close_handler: FD " << fd << ", handler=" <<
            handler << ", data=" << data);
 
-    AsyncCall::Pointer p;
-    for (p = fd_table[fd].closeHandler; p != NULL; p = p->Next()) {
+    AsyncCall::Pointer p, prev = NULL;
+    for (p = fd_table[fd].closeHandler; p != NULL; prev = p, p = p->Next()) {
         typedef CommCbFunPtrCallT<CommCloseCbPtrFun> Call;
         const Call *call = dynamic_cast<const Call*>(p.getRaw());
         if (!call) // method callbacks have their own comm_remove_close_handler
@@ -1735,9 +1735,10 @@ comm_remove_close_handler(int fd, PF * handler, void *data)
     }
 
     // comm_close removes all close handlers so our handler may be gone
-    if (p != NULL)
+    if (p != NULL) {
+        p->dequeue(fd_table[fd].closeHandler, prev);
         p->cancel("comm_remove_close_handler");
-    // TODO: should we remove the handler from the close handlers list?
+    }
 }
 
 // remove method-based close handler
@@ -1748,15 +1749,11 @@ comm_remove_close_handler(int fd, AsyncCall::Pointer &call)
     debugs(5, 5, "comm_remove_close_handler: FD " << fd << ", AsyncCall=" << call);
 
     // comm_close removes all close handlers so our handler may be gone
-    // TODO: should we remove the handler from the close handlers list?
-#if 0
-    // Check to see if really exist  the given AsyncCall in comm_close handlers
-    // TODO: optimize: this slow code is only needed for the assert() below
-    AsyncCall::Pointer p;
-    for (p = fd_table[fd].closeHandler; p != NULL && p != call; p = p->Next());
-    assert(p == call);
-#endif
+    AsyncCall::Pointer p, prev = NULL;
+    for (p = fd_table[fd].closeHandler; p != NULL && p != call; prev = p, p = p->Next());
 
+    if (p != NULL)
+        p->dequeue(fd_table[fd].closeHandler, prev);
     call->cancel("comm_remove_close_handler");
 }