From 44f726820207a535fb037ff5d81179305af15d25 Mon Sep 17 00:00:00 2001 From: Amos Jeffries Date: Sun, 5 Dec 2010 19:14:40 -0700 Subject: [PATCH] Author: Stefan Fritsch Bug 3096: Squid destroys CbDataList objects too late When server download speed exceeds client download speed, Squid creates a CbDataList 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 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. --- src/base/AsyncCall.cc | 10 ++++++++++ src/base/AsyncCall.h | 3 +++ src/comm.cc | 21 +++++++++------------ 3 files changed, 22 insertions(+), 12 deletions(-) diff --git a/src/base/AsyncCall.cc b/src/base/AsyncCall.cc index 9bca17efe8..a5eb37df7e 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 d4df9f6441..29c7f19787 100644 --- 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 b5e927f5f3..c74425e4a6 100644 --- 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 Call; const Call *call = dynamic_cast(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"); } -- 2.47.2