Re: [DRAFT][MERGE] Cleanup comm outgoing connections in trunk

Alex Rousskov wrote:
> On 05/24/2010 03:46 AM, Amos Jeffries wrote:
>> Alex Rousskov wrote:
>>> * Comm::Connection copy constructor does not copy peer correctly OR
>>> Comm::Connection destructor does not clean peer correctly. It may be a
>>> good idea to hide peer_ from public use to ensure it is always
>>> locked/unlocked properly.
>> Okay. Fixed the constructors and hidden the _peer variable behind
>> accessors.
>> One thing though, the set accessor can do cbdata ops fine, is it okay
>> for the retrieve acessor to just return the ptr? leaving
>> cbdataReference*( x.peer() ) to the calling code?
>
> Yes, it is OK. The object storing a pointer should lock/unlock it as
> needed. Accessors should not do that if they just return what was
> already stored.
>
>
>> Which is preferred in this case where the paths is only valid as
>> long as FwdStateData exists? RefCounted or cbdataReference?
>
> I prefer RefCounted if you do not need to separate "valid" from
> "allocated". Looks like that is what your second revision implements.
>
>
>> dialer makes sure the object being dialed still exists, or drops the
>> call. Am I correct?
>
> You are correct. However, when the Call object stores something, we need
> to be careful that this "something" is still valid when the Call object
> accesses it. For example, a Call or Dialer object should not print a
> call argument for debugging if that argument may be invalid. This is a
> general answer to a general question; not specific to your changes.
>
>
>> I may have overlooked somewhere where FwdStateData could be deleted by
>> operations parallel to FwdStateData::start(). Though I believe if that
>> is possible we have a terribly huge race bug somewhere.
>
>
> AsyncJobs should be started using AsyncStart() which never fails. Any
> failures inside AsyncJob::start() should be no different from failures
> inside any other part of the job that is executed asynchronously because
> the initiator code does not really know when start() will be executed.
>
> IIRC, we violate the AsyncStart rule in client_side*.cc (at least).
>
> Again, general answer here, not specific to your code.
>
>
>>> * Please remove ConnectStateData solo connections as
>>> exceptional/different code path. It has already bitten you (see below).
>>> One alternative is to create paths with a single connection instead
>>> (inside the solo constructor which will be the only solo-specific code
>>> then). Another alternative is to add path/solo access methods that hide
>>> the difference from the rest of the ConnectStateData code without the
>>> Paths creation overhead.
>> This was the first model I started with. It runs onto severe problems
>> with connections such as Ident, slow ACL, and DNS where there is no
>> state object to hold the authoritive lifetime of the paths vector.
>
> Neither of the two proposed solutions assume the presence of such an
> object (other than ConnectStateData object itself).
>
>> Its also terribly inefficient allocating a vector, adding the single
>> Comm::Connection object and pushing it, having comm navigate through
>> extra API layers to validate its presence then access it.
>
> The second solution does not allocate a vector.
>
>
>> You will have to straighten me out on this but I think it may be needed
>> by ICAP Xaction when the ICAP is configured with a service name (DNS
>> lookup plus vector of destinations assembled) instead of IP address
>> (singleton).
>
> Not sure. Do "paths" stand for multiple IP addresses with a single DNS
> name? AFAICT, it is not documented or, to be more precise, the paths
> members are documented as being ... paths.

They can do. Paths is at the IP level below the name level of DNS, so
one DNS result in multiple potential connection paths.

We don't yet expand the outgoingAddr set for each of those destinations,
but the potential is becoming present to do so in future.

>
>
>>> * Whenever possible, please declare Pointer parameters and Pointer
>>> return types as const references to avoid unnecessary locking/unlocking.
>>>
>> Unfortunately its not easy to set const on the volatile Comm::Connection
>> objects in outbound connection handling. They get created as
>> might-be-used objects and released on failures. Activating one will
>> shortly result in Comm updating it's properties (transforming wildcard
>> IP to specific on-link IPs etc).
>
> I was not talking about that. I will try to be more specific in the next
> review email.
>
>
>>> * We now have ConnectStateData and ConnStateData public classes.
>>> StateData suffix is already wonderfully descriptive, but this brings
>>> confusion to the next level.
>> Aye. I'm slowly working through comm.cc figuring out what bits are used
>> by which side. ConnStateData will hopefully get a better descriptive
>> name when its scope is clearly known.
>
>
> Let's solve this by renaming ConnectStateData instead. I believe you
> already defined ConnStateData scope. With some polishing touches I would
> restate it as:
>
> /// Async-opener of a Comm connection.
> /// Can find the first "working" among multiple destinations.
>
> If you agree with the above scope, let's s/ConnectStateData/ConnOpener/g
>
>
>>> I think the primary reason for most of these problems is that the new
>>> Connection class has dual personality. It is both holds the information
>>> necessary to establish a connection _and_ the established connection
>>> information. I suggest to split Connection into two classes:
>>>
>>> 1) Create a PeerPort or PeerAddress class that will hold information
>>> necessary to connect to a peer. This can be a cache_peer, an origin
>>> server, and ICAP server, and [in SMP branch] another Squid process.
>>> FwdState and others that need to cycle through peers will create and
>>> pass PeerPorts or PeerAddresses arrays to Comm::Connector class (see
>>> below).
>>>
>>> 2) Connection object will have a peer member of type PeerPort or
>>> PeerAddress. It will also have fd and other fields necessary for an
>>> _established_ connection.
>>>
>>> 3) Rename ConnStateData to Connector and document it as "creates a
>>> Connection to [one of] the supplied PeerPorts". This class will iterate
>>> peer details, try opening connections, and will eventually succeed or
>>> fail. When it succeeds, it will create the corresponding Connection
>>> object and return it to the caller.
>>>
>> This is back to the model we are trying to get away from.
>
> I disagree that what I am proposing is what we should get away from
> (naturally).
>
> The value of your changes is in moving the handling of connection
> retries and multiple destinations from high-level code to a dedicated
> ConnOpener class. I agree that it is the right thing to do. No argument
> there.
>
> I just think it should be done differently when it comes to handling
> possible Connection destinations. I think it is wrong to store a vector
> of unconnected Connections and then use the first element when we can
> store a vector of Destinations and use one resulting Connection instead.
>

Agreed. The Destination being IPs, we end up with an object which stores:
  destination IP + port
  intended local IP + port
  intended TCP / TLS flags
  flags for whether FD is pconn or pinned
  ptr to the peer if a peer link.

... (the fields of Comm::Connection data storage).

using two objects would call for this pathway:
   generated by peer selection, passed to forwarding,
  passed to comm, COPIED to comm with an FD number added, COPIED to
logging, then discarded.

Using Comm::Connection from template stage through to completed
connection with just an update of the FD field (instead of flag + field)
gets rid of one copy already. Next upgrade will be towards getting rid
of the second.
  Along with several other copies on the client conn.

>
>> Your PeerPort class == FwdServer (generated by peer select and handled
>> by FwdStateData when calling ConnectStateData)
>
> They are probably similar, but the current code is not using FwdServer
> for all connection establishment calls. I think it is good to have a

It is. The FwdServer fields get de-constructed into parameters for
multiple comm_* functions in forwarding. Which permeate through comm to
an fde piecemeal.

> single Connection Destination class(es) that all code is using in a
> uniform fashion. Your code does that, but using the wrong class
> (full-blown Connection instead of small and specific Destination).
>
>
>>> * Will Comm::Connection be used for incoming connections as well? If
>>> not, the name is probably too generic. Should be something like
>>> PeerConnection. Or perhaps we do not need this class at all?! If it is
>>> only used as an information holder during callback (after the above
>>> PeerPort changes) then the callback data should be modified to carry
>>> that information instead of adding a new class.
>>>
>> see comm flow description above.
>
>
> I still think the above concern is valid. I hope that the problems with
> the current implementation (a collection of unconnected connections) are
> clear enough. I tried to clarify above that I am not proposing to going
> back to "handle everything in high-level code" model. I only want to
> change the connection destination type (solo and paths) from
> "unconnected Connection" to "Connection Destination".

We have a miss-communication here.
You describe a concern:

>>> * Will Comm::Connection be used for incoming connections as well?

Answer being: Yes it is used for incomming.

>>> If not, the name is probably too generic. Should be something like
>>> PeerConnection.

Therefore renaming (on these particular grounds) is not grounded.

>>> Or perhaps we do not need this class at all?! If it is
>>> only used as an information holder during callback (after the above
>>> PeerPort changes) then the callback data should be modified to carry
>>> that information instead of adding a new class.

There are several different ways this same identical data is being used,
fields of ConnStateData, fde, HttpRequest meta fields, AccessLogEntry
fields. function parameters passed around piece-meal with some complex
lookups needed when any given piece of the data is not passed over some
middle section of code (comm_local_port() etc).

This is an aim to normalize the particular and small TCP link data
end-to-end across the squid processing pathways without copying.

I have not yet fully merged fde in yet, but when done the fd_table can
be a table/vector/map of pointers to Comm::Connection which have been
opened.

>
> Review of your latest patch will follow in a few hours.

Responding to particular changes in that emails response.

Amos

-- 
Please be using
   Current Stable Squid 2.7.STABLE9 or 3.1.4