Re: [PATCH] Comm cleanup (Async ConnOpener)

On 07/18/2010 08:34 PM, Amos Jeffries wrote:
> On Sun, 18 Jul 2010 19:15:37 -0600, Alex Rousskov
> <rousskov_at_measurement-factory.com> wrote:
>
>> * Can Adaptation::Icap::Xaction::closeConnection set connection to NULL?
>>
>
> Yes.
>
> Since I got to know AsyncJob a bit better now I think the concept of
> "connection" pointer in Xaction is maybe a little wrong. Check me on this
> but here are a few state things I found working with FwdState and the
> InternalServer from the PAC stuff...
>
>
> In the Xaction code "connection" is meant to indicate both that a
> connection is being attempted and what that connection is (its FD). right?

If you are talking about the old (unpatched) code, then the non-negative
connection member indicates that we own a connection to the ICAP server.
The exact origin (reused or new) or state (opening, open, closing) of
that connection is not important, in general.

> In the async time between connect being started and connect being
> completed the FD-state of connection switches from being <0 to being >=0.

In the old code, that was not happening. In the patched code, yes.

> This starts a race condition between the ConnOpener finishing and any
> caller abort/timeout handler which may try to close an half-open conn->fd
> itself. Which would trigger the ConnOpener to open a new socket on it's
> next call and re-start the connection setup, then die on its AsyncCall
> callback (for some reason the dialer GetParams fails with a NULL params
> object if the caller has died but the call is still scheduled without
> cancelling).

One more reason why dual-use Connection/Destination is a bad idea.

> The solution I use in the ConnOpener itself (for earlyabort and timeout)
> is to have the caller hold a AsyncCall::Pointer to the call is told the
> ConnOpener to make. And use that ptrs NULL / not-NULL state in the abort
> code to identify if a ConnOpener is running or not. IF a ConnOpener is
> running then the call (for which we now have a ptr) can be cancelled and
> the conn 'erased' (Comm::ConnectionPointer set to NULL along with the held
> AsyncCall::Pointer). This leaves the ConnOpener to finish and no callback
> happens, ConnOpener completing then re-closes the FD which was attempted.

ICAP::Xaction already maintains the opener callback called "connector".
Feel free to change the code so that the connector and fd are
interpreted together: if there is a connector, there is no fd. If there
is no connector but there is connection pointer, the fd can be extracted
from the connection. Just try not to change the old semantics of various
checks.

HTH,

Alex.
Received on Mon Jul 19 2010 - 15:55:31 MDT