Re: ICAP connections under heavy loads

On 8/09/2012 3:17 a.m., Alex Rousskov wrote:
> On 09/07/2012 08:33 AM, Alexander Komyagin wrote:
>> On Fri, 2012-09-07 at 08:15 -0600, Alex Rousskov wrote:
>>> On 09/07/2012 02:32 AM, Alexander Komyagin wrote:
>>>> However, as I stated earlier, the comm.cc problem (actually semantics
>>>> problem) persists. I think it should be documented that second and
>>>> subsequent calls to comm_connect_addr() do not guarantee connection
>>>> establishment unless there was a correct select() notification.
>>> Agreed: We should document what comm_connect_addr() does. However, does
>>> it provide any guarantees even _after_ select() notification? According

Which to me means we cannot rely on it at all for the timeout callback
cases - which have no knowledge of whether select() notification has
occured. The race condition in question being the race between select()
notification and timout notification arriving.

The entire point of calling comm_connect_addr() from timeout callback is
to determine whether select() notification is in flight. If that fails
(as it does for your cases) it is useless doing, we may as well treat
them all as timeouts and abort quickly.

>>> to Stevens, the current code may still report that there is no problem
>>> when in fact there is one (and we will detect it later during I/O), right?
>> After select() notification there are two scenarios possible:
>> 1) connection was successfully established (we got EPOLLOUT there). Then
>> getsockopt() would correctly report success;
>> 2) there was a problem (we got EPOLLHUP or EPOLLERR). In this case
>> getsockopt() shall report an error (i.e. appropriate errno).
>>
>> So I think we can rely on comm_connect_addr(), but only _after_ the
>> notification.

That sequence of cases is only reachable from the
InProgressConnectRetry() callback handlers which schedule connect()
directly after select() write notfication.

The timeout handler (and also DelayedConnectRetry() handler) are dealing
with the inverse of those cases, where select() notification and
followup async call to trigger connect() are still in-flight or have not
happened at all.

I think from all this discussion the ConnOpener::DelayedConnectRetry
handler needs to be investigated as well. It will hit the same error
cases, but having it schedule timeout() instead of connect() is wrong -
since it is for re-trying the connect status check before BOTH timeout
and select() notifications have happened.

>
> This is not how I understand the comm_connect_addr() call context.
> comm_connect_addr() is called by ConnOpener in at least two cases:
>
> 1. To initiate the connection, after comm_openex in ConnOpener::start().
ConnOpener::connect() - called synchronously from start().

> 2. After select(2) or equivalent said that the socket is ready for writing.

This is connect() scheduled from ConnOpener::InProgressConnectRetry().

  ** we could cancel the timeout and earlyAbort calls in the wrapper
when scheduling the connect() call to happen later. This will reduce the
async lag when those might kick in.

3. after system-connect(), before timeout or select() notification is
received

This is ConnOpener::DelayedRetryConnectRetry()

   * select maybe ready or not, timeout may also be pending or not. Thus
the if() timeout checks inside connect() swicth cases to abort on timeout.

4. on timeout notification before select() notification is received
(maybe ready, maybe not)

This is ConnOpener::timeout().

  * only the OK switch case from connect() is useful, all other events
COMM_TIMOUT callback notice needs to be sent.

>
>
> Let's focus on the second context. We know the socket is writeable. This
> can mean that either (2a) the connection was established (or at least
> ready for writing) OR (2b) that there was a problem establishing the
> connection.
>
> Can a getsockopt() call in case 2b guarantee problem detection? If my
> reading of Stevens explanation is correct, it cannot (because Stevens
> suggests three ways to get that guarantee instead of calling getsockopt,
> implying that getsockopt is not reliable in this context).
>
> If I am right, the new documentation should reflect this uncertainty.
> However, as discussed before, the higher level code will still work even
> if we do not notice the error right away (we will get an error when we
> try to write or read).
>
>
> HTH,
>
> Alex.
>
Received on Sat Sep 08 2012 - 03:51:54 MDT