Re: [PATCH] Comm::Connection and friends

On 03/26/2011 05:19 AM, Amos Jeffries wrote:
> I've already split out what I can from the cleanup branch and submitted
> separately. This patch consists of the remaining changes which are hard
> to separate.
>
> In general overview:
> The premise underlying this whole patch is that instead of copying and
> re-copying and re-lookups for the FD related data we can take the
> ConnectionDetail class which is generated to store a few bits of IP
> information about newly accept()'d connections and make it persist
> across the whole of Squid.
>
> It has been renamed from ConnectionDetails to Comm::Connection and has
> absorbed a few FD data fields from other classes long the code paths.
> Its scope is to hold an FD (or potential FD) plus meta data.
>
> Comm::Connection are valid before, during and after the period when
> their particular FD is open. The meta data may be used beforehand to
> setup the FD (in the case of peer FDs), and it may remain in use after
> FD closure until logging or all linked job and call objects have
> detected the closure and terminated. A global function
> Comm::IsConnOpen() may be used on the pointer objects to detect whether
> they point at an active connection.
>
> Most of the patch is simple parameter changes to functions and methods
> to pass a "cont Comm::ConnectionPointer &" instead of an "int FD". Along
> with class FD fields being converted to these object pointers.
>
>
> In order to support this alteration there have been behavioural changes to:
>
> the socket accept() Job
> Comm::TcpAcceptor altered to spawn Comm::Connection objects and to
> operate with one controlling their active/closed state.
>
> FTP data channel handling Calls.
> efficiency improvements making use of Comm::Connection as a feedback
> channel between TcpAcceptor and FtpStateData to cancel the listening
> Job. Most of the underlying logic change is already in trunk to use the
> Subscription API.
>
> Peer selection
> updated to spawn a set of Comm::Connection objects. To do this it is
> updated to determine *all* peers including DIRECT ones. Doing the DNS
> lookup instead of leaving it to comm_connect() on the other side of
> FwdState. It also absorbs the outgoing address selection from FwdState
> and can now specify details of local+remote ends of an outgoing TCP link.
>
> Forwarding
> updated to handle the new outputs from peer selection and to open
> sequentially.
>
> pconn handling
> updated to use destination IP/port and hold a Comm::Connection
> instead of domain name indexing an FD. This allows us to maintain idle
> pools and re-use FD more efficiently with virtual-hosted servers. Along
> with maintaining certainty that the pconn selected actually goes to the
> exact destination IP:port needed by forwarding.
> (unchanged since it was audited individually earlier)
>
> comm layer outgoing connections
> now have a control job Comm::ConnOpener to do this. Due to the peer
> selection and forwarding changes this is a much simpler operation.
> (unchanged since it was audited individually earlier)
>
> http / tunnel / gopher / whois / ftp
> updated to receive a server and client Comm::Connection object from
> forwarding. To operate on those until they close or are finished with.
> (they are a step or two away from being fully polished independent
> async jobs, I consider that outside this patch scope).
>
> SNMP / ICP / HTCP / DNS port listeners
> updated to work with Comm::Connection holding their listening socket
> meta data. This is a side-effect of the ICP and Comm read/write/timeout
> changes.
>
>
>
> This patch closes the following bugs:
> http://bugs.squid-cache.org/show_bug.cgi?id=2561
> http://bugs.squid-cache.org/show_bug.cgi?id=3050
> http://bugs.squid-cache.org/show_bug.cgi?id=3177
>
> By passing FD meta data as Comm::Connection it forms the groundwork to
> also close these bugs quite easily:
> http://bugs.squid-cache.org/show_bug.cgi?id=2279
> http://bugs.squid-cache.org/show_bug.cgi?id=692
> http://bugs.squid-cache.org/show_bug.cgi?id=2558
>
> I believe these are also fixed or nearly so, but have not tested. They
> fall into one of the two categories above:
> http://bugs.squid-cache.org/show_bug.cgi?id=3108
> http://bugs.squid-cache.org/show_bug.cgi?id=3045

How about adding a unique ID to Comm::Connection so that we can track
connections better in debugging logs, despite FD number and Connection
memory recycling? Our of scope?

Please s/dataDescriptor/primaryConnection/ or something like that.

s/for teh purpose/for the purpose/

> + * While the properties may be changed, this is for teh purpose of creating
> + * potential connection descriptors which may be opened. Properties should
> + * be considered read-only outside of the Comm layer code once the connection
> + * is open.

I do not quite get the first sentence, especially the "potential
connection descriptors which may be opened" part. Replace with something
like "Connection properties may be changed until the connection is opened"?

> + /** standard empty connection creation */

I do not think a "standard empty connection" defines anything. Remove or
replace with something like "creates a not-opened connection with
default properties"?

> + /** Socket used by this connection. -1 if no socket has been opened. */
> + int fd;

Also -1 if the socket was closed. Replace with "Negative if not open"?

> + Comm::ConnectionPointer nul;
> + FwdState::fwdStart(nul, ...

s/nul/nil/ or s/nul/null/ to use an existing word. You may also be able
to just use Comm::ConnectionPointer() as a parameter instead.

The patch is too long for me to review fully, unfortunately. I
understand that portions of it were reviewed earlier, but I cannot
easily exclude those portions from the patch so it does not really help.

Several changes may have a negative performance effect (e.g., memory
pools for persistent connection lists are gone?). There are also many
rather complex changes that might change functionality even if we do not
expect it. If this patch is committed now, are you going to port it to
the v3.2 branch or is it a v3.3-only change?

Thank you,

Alex.
Received on Tue Mar 29 2011 - 18:11:44 MDT