From: Alex Rousskov <rousskov@measurement-factory.com>
Date: Thu, 2 Dec 2010 23:33:27 +0000 (-0700)
Subject: Author: Stefan Fritsch <sf@sfritsch.de>
X-Git-Tag: take1~50
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=37cba319026e2bd0dd323fb01182f1dda8cbe593;p=thirdparty%2Fsquid.git

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 8b241f3461..f144bbab9d 100644
--- 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 01066ca2ce..12da25a733 100644
--- 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 7cc28e5b7a..7fe3c10826 100644
--- 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");
 }