Re: [PATCH] Comm cleanup (Async ConnOpener)

On Sun, 18 Jul 2010 19:15:37 -0600, Alex Rousskov
<rousskov_at_measurement-factory.com> wrote:
> On 07/18/2010 04:52 AM, Amos Jeffries wrote:
>> This updated my previous submission for the comm connection opener
model.
>>
>> diff since last time:
>>
>> - removed the multi-path pathways. They are now isolated at the upper
>> level. ConnOpener now handles only one Comm::Connection.
>>
>> - implemented ConnOpener as an AsyncJob child.
>>
>>
>> What has changed from HEAD:
>>
>> ConnectionDetails objects have been renamed Comm::Connection and been
>> extended to hold the FD and Squids' socket flags.
> ...
>> On COMM_ERR_CONNECT and COMM_TIMEOUT the conn will be !isOpen() or
NULL
>> and should not be relied on.
>
> If the connection pointer is NULL, a job opening multiple connections
> would not be able to tell which connection just failed. Let's not hide
> connection pointer from the initiator.

Understood. I'm not sure exactly why I wrote NULL as a case there.

>
> * 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?

In the async time between connect being started and connect being
completed the FD-state of connection switches from being <0 to being >=0.
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).

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.

<snip bits I'll reply to when I'm back in front of the code>
>
> * FwdState::complete may shift paths (via reforward). If that happens,
> the following will close a different/new connection or assert/throw (or
> even segfault in the current patch):
>
> fwd->complete();
> - comm_close(fd);
> + fwd->conn()->close();
>
> Overall, exposing connection pointer to Servers seems wrong, especially
> because the pointer may change from one call to another. A Server that
> needs that pointer, should store it at initialization time, just like it
> stores fd in the current code.
>
> You have "TODO: store Comm::Connection instead of FD". This is an XXX
> and not just a TODO because of the above concerns.

Ah, so XXX to you means something other than a personal TODO marker?

<snip>
>
>
> That is all I had time for, sorry.
>

No prob. thats heaps for me to work on tonight :)
Received on Mon Jul 19 2010 - 02:34:28 MDT