Re: ICAP connections under heavy loads

On Wed, 2012-09-05 at 09:59 -0600, Alex Rousskov wrote:
> On 09/05/2012 09:27 AM, Alexander Komyagin wrote:
>
> > So you think that it's ok for comm_coonect_addr() to return COMM_OK if
> > it was called before the appropriate select() notification. Am I right?
>
> Hard to say for sure since comm_coonect_addr() lacks an API description,
> and there are at least three similar but different ways the function is
> being used by Squid.
>
> One natural way to define this API is to say that it should return
> COMM_OK if and only if the socket is connected (making any select
> notifications irrelevant). However, this definition may be too
> CPU-expensive and/or too unportable to support. And this level of
> certainty may not actually be needed for current Squid needs!

Maybe you're right, Alex. However, I would prefer this function to have
the very strict semantics: it should return COMM_OK if and only if the
socket is connected (just like you said). Because this way any
upper-level code can rely on it.

Otherwise subsequent calls to comm_connect_addr() would in fact just
check if there are any errors on pipe, and thus we got controversy with
the function name (shall we ask the guy who introduced it? :) ).

>
> I would not be surprised if there is some gray area where we cannot
> really tell for sure (without too much additional overhead or
> portability risk) whether the async socket is connected. The Stevens
> book seem to imply that much. Inside that gray area, the function
> should probably return COMM_OK so that the rest of the code works: If we
> guessed wrong, we will get failures during I/O, but the code has to deal
> with those anyway.

I guess those I/O failures would cost us CPU cycles and make connection
problems very-very-very hard to debug. Also all connection timeouts
become useless if we can't tell for sure whether the socket is really
connected.

>
> In other words, if you want to work on this, consider defining the API
> based on current Squid needs and then make sure we support those minimum
> requirements, keeping overheads and portability in mind. However, I
> would _start_ by fixing the calling code first (as it may affect the
> minimum requirements) -- your ConnOpener patch was a step in that direction.

Amos wrote that calling code is actually OK. I agree. Still we could fix
the proper semantics (and put it into doxygen) for comm_connect_addr and
CommOpener::connect() to avoid future misunderstanding and debates.

For instance, comm_connect_addr() and all it derivatives provides
non-blocking socket connect request, returning COMM_OK iff connection
was successfully established, COMM_INPROGRESS if it's still scheduled to
complete, or {COMM_ERROR|COMM_ERR_PROTOCOL} if errors were encountered.

>
>
> HTH,
>
> Alex.
>
>
> > On Wed, 2012-09-05 at 08:30 -0600, Alex Rousskov wrote:
> >> On 09/05/2012 03:32 AM, Alexander Komyagin wrote:
> >>> On Tue, 2012-09-04 at 09:16 -0600, Alex Rousskov wrote:
> >>
> >>>> Again, I hope that this trick is not needed to solve your problem, and I
> >>>> am worried that it will cause more/different problems elsewhere. I would
> >>>> recommend fixing CommOpener instead. If _that_ is not sufficient, we can
> >>>> discuss appropriate low-level enhancements.
> >>>
> >>> Both things shall be fixed, IMHO.
> >>
> >> If double-connect is not needed, we should not introduce it. AFAICT,
> >> double-connect is an attempt to cope with a bug in higher-level code. We
> >> should fix that bug and see if that is sufficient. If it is not
> >> sufficient, we should evaluate why and only then add appropriate
> >> low-level code if needed.
> >>
> >> The primary goal here is to fix the underlying issue, not just to find a
> >> workaround (which you have already provided).
> >>
> >>
> >> Thank you,
> >>
> >> Alex.
> >>
> >
>

-- 
Best wishes,
Alexander Komyagin