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

On 17/07/2014 11:10 a.m., Alex Rousskov wrote:
> 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.

Looks like a reasonable temporary solution to me for the stable
releases. I have no problems with it going in if we continue to work on
the queue fixes for trunk.

Regarding that queue I think it can be cleaned up once and for all by
making doAccept() a proper Call and using the new comm/Read.h API. The
queue becomes a std::queue<AsyncCall::Pointer> and the nasty
removeDead() drops in favour of a TcpAcceptor internal Call::cancel().
No more Job raw pointers!
 NP that this change will not fix the stable releases due to missing API.

Amos
Received on Thu Jul 17 2014 - 00:47:05 MDT