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 8b241f34613103b650c18ecc74a10a5b9964f8d1..f144bbab9d91417768109ae23c52740e55c386aa 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 01066ca2ce3b4d05af23c468103a47efdd60d4b2..12da25a733d92dd4526acb09632dbd08b538a3cd 100644 (file)
--- a/src/base/AsyncCall.h
+++ b/src/base/AsyncCall.h
@@ -58,6 +58,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 7cc28e5b7a6ec31f598187e6c5f0770ddb35dd99..7fe3c108265a529500d0d068315d916a25e3a576 100644 (file)
--- a/src/comm.cc
+++ b/src/comm.cc
@@ -1578,8 +1578,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
@@ -1592,9 +1592,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
@@ -1605,15 +1606,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");
 }