Re: [PATCH] Cleanup comm connection setup layering

On 06/24/2010 10:21 PM, Amos Jeffries wrote:
> Alex Rousskov wrote:
>> On 06/09/2010 05:44 AM, Amos Jeffries wrote:
>>> (since last submission I've implemented most of Alex suggestions
>>
>>
>> At the design level, the following problems need to be addressed, IMO:
>>
>> Connection dual-use: Replace unconnected connections with some sort of
>> connection destination class and use it to configure the connection
>> opener (ConnectStateData) object for solo and paths cases. The
>> connection opener should return a single working Connection object on
>> success (among other things).
>
> Um, one of the middling-important goals here is to reduce copying of the
> comm data properties from one alloc'd object to another and stack.
> Preserving the handles (refcount pointer) already stored by callers
> (FTP, DNS for starters).
> I'm happy to make a typedef to differentiate the scopes for
> readability. Though it seems to me that will just make more complexity.

If Destinations are stored using pointers (which is what you already
have for Connections so no change here), then there is no extra copying.
In addition to that, I disagree that avoiding a few copies is actually
important here. The true value of your work is elsewhere, even if you
think you are doing performance optimizations :-).

BTW, this is still valid after you move the paths handling elsewhere.

>>> Peer selection has been extended to do DNS lookups on the peers chosen
>>> for forwarding to and produce a vector<> of possible connection
>>> endpoints (squid local IP via tcp_outgoing_address or tproxy) and remote
>>> server.
>>>
>>> ConnectStateData has been moved into src/comm/ (not yet namespaced) and
>>> had all its DNS lookup operations dropped. To be replaced by a looping
>>> process of attempting to open a socket and join a link as described by
>>> some Comm::Connection or vector<> of same.
>>> Limited by connect_timeout in its running time.
>>> Extended by connect_retries number of attempts to open each path.
>>
>> Can ConnectStateData handle DNS lookups? Does the peer selection
>> algorithm need tight control over those lookups before we try to open a
>> connection to a peer?
>
> No, ConnectStateData doing its own DNS was the v4/v6 block on
> split-stack IPv6 support for BSD. It blocks outgoing address selection
> based on higher scoped HttpRequest details.

Sorry, I do not really understand the above, but it would be nice if
connection opener could handle async DNS lookups because then all
higher-level code would not have to deal with DNS. This is not in your
original project scope, and is not a blocker.

> If the requestor disappears and everything goes pear shaped the
> Comm::Connection has just enough smarts to close the FD and prevent
> leaks in its own destructor.

> In the case where the original requestor has died then ConnectStateData
> or dialer parameter details is left the only holder of the
> ConnectionPointer and FD closure will be actioned when that state gets
> dropped by the dialer.

Right, sounds good.

> We still have the case where an object holding a Comm::Connection
> pointer leaks at a higher level. But there is nothing extra comm can do
> about that.

Old bugs in higher-level objects are outside your patch scope, IMO.

>> * Please use "const reference" types for Pointer methods parameters by
>> default. This avoids needless locking/unlocking during parameter
>> copying. For example,
>>
>> ConnectStateData(Comm::PathsPointer paths, AsyncCall::Pointer handler)
>>
>> can be optimized as
>>
>> ConnectStateData(const Comm::PathsPointer &paths, const
>> AsyncCall::Pointer &handler)
>
> Methods okay I understand.
> That example is a bad choice though, the constructor needs to lock
> references.

I do not know what "lock references" means, but does not the constructor
just copies Pointers to local members? If yes, then const references
would eliminate useless locking/unlocking.

>> * ConnectStateData is not owned by anyone, deletes self, and is
>> scheduling/receiving async calls. It should be converted into an
>> AsyncJob with proper start/done methods. As a free bonus, this will
>> greatly help with debugging.
>
> Sweet. Underway.
>
> When I get my head around the Dialer parameters usage, those
> ConnectCbFunPtr will die as well.

Yes, static callback wrappers should not be created in new code. See
Icap::Xaction for working examples without static wrappers. You do not
need to create any new dialer classes for existing comm calls.

>> * In the code below, can we set the connection pointer to NULL instead
>> of dragging it (and the connection object) around after
>> closeConnection()?
>>
>>> writer = NULL;
>>> reader = NULL;
>>> connector = NULL;
>>> - connection = -1;
>>> }
>>> }
>
> Particular references can be set to NULL if no longer needed. Just be
> careful of logging, which often needs things long after the active code.
>
> Setting an AccessLogEntry field and unsetting connection local field is
> probably the way to go.

AFAICT, if the old code could get rid of the connection at that point,
your code should be able to do the same without making things worse. On
the other hand, simply removing that old code leaves Xaction in an
inconsistent "closed but have connection" state.

>> * Can the new code below be moved after the Xaction state is checked and
>> properly synced? The current implementation leaves Xaction in an
>> inconsistent state: we are connecting and the connection have timed out.
>>
>>> + if (io.flag == COMM_TIMEOUT) {
>>> + handleCommTimedout();
>>> + return;
>>> + }
>>> +
>>> Must(connector != NULL);
>>> connector = NULL;
>
> Inconsistent? That is the state.
> We are connecting and the connection attempt has taken too long and
> already closed.

Yes, the above code is correct, sorry. The timeout handler actually
needs the connector member to be set. I forgot about that hack. To
clarify, please:

* Add a "// leave connector set to indicate connect() timeout" commend
above or to the left of the handleCommTimedout() call.

Thank you,

Alex.
Received on Fri Jun 25 2010 - 23:41:11 MDT