Comm::TcpAcceptor::doAccept fd limit handling is broken

NB: This occurred in the real world on some 'customer machine' using
3.3.12. Since the code is unchanged in 3.HEAD, I assume it is affected,
too.

The method mentioned in the subject is

void
Comm::TcpAcceptor::doAccept(int fd, void *data)
{
    try {
        debugs(5, 2, HERE << "New connection on FD " << fd);

        Must(isOpen(fd));
        TcpAcceptor *afd = static_cast<TcpAcceptor*>(data);

        if (!okToAccept()) {
            AcceptLimiter::Instance().defer(afd);
        } 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());
    } catch (...) {
        fatal("FATAL: error while accepting new client connection: [unkown]\n");
    }
}

This is broken because it re-enables readable notifications even if no
connection was accepted. Since the socket will remain readable while a
connection is pending, it will thus keep queueing the same afd until an
already accepted connection is closed. With some bad luck, this can end
up as billions of queued nothings which then cause the proxy to become
very obsessed with itself for a long time thanks to the O(n * n) shift
operation performed by AcceptLimiter::kick. Judging from what I've seen
from the code, the deferred queue could probably be ripped
completely. More conservative change which fixes this for my test case
(code modified to accept only one connection at the same time):

NB^2: This is an attempt to be polite and helpful, not an invitation to
use me for target practice.

--- mad-squid/src/comm/TcpAcceptor.cc 8 Jan 2013 17:36:44 -0000 1.1.1.2
+++ mad-squid/src/comm/TcpAcceptor.cc 16 Jul 2014 16:51:25 -0000
@@ -204,7 +204,6 @@
         } 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());
@@ -257,7 +256,8 @@
         notify(flag, newConnDetails);
         mustStop("Listener socket closed");
         return;
- }
+ } else if (!isLimited)
+ SetSelect(conn->fd, COMM_SELECT_READ, Comm::TcpAcceptor::doAccept, this, 0);
 
     debugs(5, 5, HERE << "Listener: " << conn <<
            " accepted new connection " << newConnDetails <<
Received on Wed Jul 16 2014 - 17:11:48 MDT