Re: [PATCH] Comm::TcpAcceptor::doAccept fd limit handling is broken

On 07/16/2014 02:38 PM, Rainer Weikusat wrote:
> Alex Rousskov <rousskov_at_measurement-factory.com> writes:
>> On 07/16/2014 11:11 AM, Rainer Weikusat wrote:
>>> This is broken because it re-enables readable notifications even if no
>>> connection was accepted.

>> In other words, it re-enables a check for new connections when
>>
>> * we know that we currently cannot accept any new connections,
>> * Squid is under stress already, and
>> * our code does not handle multiple deferences of the same TcpAcceptor
>> object well.

> There's an important 4th point: The check is re-enabled when it is known
> that it will immediately return true again because the connection which
> was not accepted is still pending (hence, the socket is still readable),
> more details below.

Agreed!

> I changed the TcpAcceptor/ AcceptLimiter code to act as if
> only a single file descriptor was available for client connections and
> created two connections via netcat. What it should do in this case is to
> ignore the second connection until the first one was closed. Instead, it
> went into an endless 'defer this connection' loop, as mentioned above,
> because the pending 2nd connection meant the socket remained readable.

OK, I think I now better understand what you meant by "one connection at
the same time". And even if I do not, we seem to agree on the fix to
solve the problem, which is even more important :-).

> updated patch:
>
> --- 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 20:36:35 -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());
> @@ -262,6 +261,8 @@
> debugs(5, 5, HERE << "Listener: " << conn <<
> " accepted new connection " << newConnDetails <<
> " handler Subscription: " << theCallSub);
> +
> + if (!isLimited) SetSelect(conn->fd, COMM_SELECT_READ, Comm::TcpAcceptor::doAccept, this, 0);
> notify(flag, newConnDetails);
> }

I am happy to commit the above (after splitting the new if-statement
into two lines), but since this is not an emergency, I would prefer to
wait a little for any objections or second opinions.

Thank you,

Alex.
Received on Wed Jul 16 2014 - 23:10:41 MDT