Bug 4885: Excessive memory usage when running out of descriptors (#291)

TcpAcceptor now stops listening when it cannot accept due to FD limits.
We also no longer defer/queue the same limited TcpAcceptor multiple
times. These changes prevent unbounded memory growth and improve
performance of Squids running out of file descriptors. They should have
no impact on other Squids.

diff --git a/src/comm/AcceptLimiter.cc b/src/comm/AcceptLimiter.cc
index 26412fc1da625a6c1fb15b7116c3e42c19265590..9e0845e293e78844b31fdb28104bd5773a108b24 100644 (file)
--- a/src/comm/AcceptLimiter.cc
+++ b/src/comm/AcceptLimiter.cc
@@ -24,42 +24,33 @@ Comm::AcceptLimiter::Instance()
 void
 Comm::AcceptLimiter::defer(const Comm::TcpAcceptor::Pointer &afd)
 {
-    ++ (afd->isLimited);
-    debugs(5, 5, afd->conn << " x" << afd->isLimited);
+    debugs(5, 5, afd->conn << "; already queued: " << deferred_.size());
     deferred_.push_back(afd);
 }
 
 void
 Comm::AcceptLimiter::removeDead(const Comm::TcpAcceptor::Pointer &afd)
 {
-    uint64_t abandonedClients = 0;
-    for (unsigned int i = 0; i < deferred_.size() && afd->isLimited > 0; ++i) {
-        if (deferred_[i] == afd) {
-            -- deferred_[i]->isLimited;
-            deferred_[i] = NULL; // fast. kick() will skip empty entries later.
-            debugs(5, 5, afd->conn << " x" << afd->isLimited);
-            ++abandonedClients;
+    for (auto it = deferred_.begin(); it != deferred_.end(); ++it) {
+        if (*it == afd) {
+            *it = nullptr; // fast. kick() will skip empty entries later.
+            debugs(5,4, "Abandoned client TCP SYN by closing socket: " << afd->conn);
+            return;
         }
     }
-    debugs(5,4, "Abandoned " << abandonedClients << " client TCP SYN by closing socket: " << afd->conn);
+    debugs(5,4, "Not found " << afd->conn << " in queue, size: " << deferred_.size());
 }
 
 void
 Comm::AcceptLimiter::kick()
 {
-    // TODO: this could be optimized further with an iterator to search
-    //       looking for first non-NULL, followed by dumping the first N
-    //       with only one shift()/pop_front operation
-    //  OR, by reimplementing as a list instead of Vector.
-
     debugs(5, 5, "size=" << deferred_.size());
-    while (deferred_.size() > 0 && fdNFree() >= RESERVED_FD) {
+    while (deferred_.size() > 0 && Comm::TcpAcceptor::okToAccept()) {
         /* NP: shift() is equivalent to pop_front(). Giving us a FIFO queue. */
         TcpAcceptor::Pointer temp = deferred_.front();
         deferred_.erase(deferred_.begin());
         if (temp.valid()) {
             debugs(5, 5, "doing one.");
-            -- temp->isLimited;
             temp->acceptNext();
             break;
         }

diff --git a/src/comm/AcceptLimiter.h b/src/comm/AcceptLimiter.h
index f0b9e9eca9438a7d082014bc4e78b6106a728025..b94ae308ec89e260d356a6b9b0944a6e5fcc72f5 100644 (file)
--- a/src/comm/AcceptLimiter.h
+++ b/src/comm/AcceptLimiter.h
@@ -11,7 +11,7 @@
 
 #include "comm/TcpAcceptor.h"
 
-#include <vector>
+#include <deque>
 
 namespace Comm
 {
@@ -26,16 +26,6 @@ namespace Comm
  * removeDead - used only by Comm layer ConnAcceptor to remove themselves when dying.
  * kick - used by Comm layer when FD are closed.
  */
-/* TODO this algorithm can be optimized further:
- *
- * 1) reduce overheads by only pushing one entry per port to the list?
- * use TcpAcceptor::isLimited as a flag whether to re-list when kick()'ing
- * or to NULL an entry while scanning the list for empty spaces.
- * Side effect: TcpAcceptor->kick() becomes allowed to pull off multiple accept()'s in bunches
- *
- * 2) re-implement as a std::queue instead of std::vector
- * storing head/tail pointers for fast push/pop and avoiding the whole shift() overhead
- */
 class AcceptLimiter
 {
 
@@ -56,7 +46,7 @@ private:
     static AcceptLimiter Instance_;
 
     /** FIFO queue */
-    std::vector<TcpAcceptor::Pointer> deferred_;
+    std::deque<TcpAcceptor::Pointer> deferred_;
 };
 
 }; // namepace Comm

diff --git a/src/comm/TcpAcceptor.cc b/src/comm/TcpAcceptor.cc
index ca009ff6d38008b220638312c605897c92915ebc..7309505413f35b07bd453aca4f1ad4f5d7664e7d 100644 (file)
--- a/src/comm/TcpAcceptor.cc
+++ b/src/comm/TcpAcceptor.cc
@@ -43,7 +43,6 @@ CBDATA_NAMESPACED_CLASS_INIT(Comm, TcpAcceptor);
 Comm::TcpAcceptor::TcpAcceptor(const Comm::ConnectionPointer &newConn, const char *, const Subscription::Pointer &aSub) :
     AsyncJob("Comm::TcpAcceptor"),
     errcode(0),
-    isLimited(0),
     theCallSub(aSub),
     conn(newConn),
     listenPort_()
@@ -52,7 +51,6 @@ Comm::TcpAcceptor::TcpAcceptor(const Comm::ConnectionPointer &newConn, const cha
 Comm::TcpAcceptor::TcpAcceptor(const AnyP::PortCfgPointer &p, const char *, const Subscription::Pointer &aSub) :
     AsyncJob("Comm::TcpAcceptor"),
     errcode(0),
-    isLimited(0),
     theCallSub(aSub),
     conn(p->listenConn),
     listenPort_(p)
@@ -233,7 +231,6 @@ Comm::TcpAcceptor::doAccept(int fd, void *data)
         } else {
             afd->acceptNext();
         }
-        SetSelect(fd, COMM_SELECT_READ, Comm::TcpAcceptor::doAccept, afd, 0);
 
     } catch (const std::exception &e) {
         fatalf("FATAL: error while accepting new client connection: %s\n", e.what());
@@ -310,6 +307,7 @@ Comm::TcpAcceptor::acceptOne()
            " accepted new connection " << newConnDetails <<
            " handler Subscription: " << theCallSub);
     notify(flag, newConnDetails);
+    SetSelect(conn->fd, COMM_SELECT_READ, doAccept, this, 0);
 }
 
 void

diff --git a/src/comm/TcpAcceptor.h b/src/comm/TcpAcceptor.h
index 643773136e26b4665f1fba4003d730e22b51dbb2..0c77b0df4f1f0b2ef817d03348376c549d1da39c 100644 (file)
--- a/src/comm/TcpAcceptor.h
+++ b/src/comm/TcpAcceptor.h
@@ -76,9 +76,12 @@ public:
     /// errno code of the last accept() or listen() action if one occurred.
     int errcode;
 
+    /// Method to test if there are enough file descriptors to open a new client connection
+    /// if not the accept() will be postponed
+    static bool okToAccept();
+
 protected:
     friend class AcceptLimiter;
-    int32_t isLimited;                   ///< whether this socket is delayed and on the AcceptLimiter queue.
 
 private:
     Subscription::Pointer theCallSub;    ///< used to generate AsyncCalls handling our events.
@@ -93,10 +96,6 @@ private:
     /// listen socket closure handler
     AsyncCall::Pointer closer_;
 
-    /// Method to test if there are enough file descriptors to open a new client connection
-    /// if not the accept() will be postponed
-    static bool okToAccept();
-
     /// Method callback for whenever an FD is ready to accept a client connection.
     static void doAccept(int fd, void *data);