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

Alex Rousskov wrote:
> Hi Amos,
>
> Here are a few comments regarding comm outgoing connections cleanup.
> There are several high-level issues that need to be addressed and a few
> low-level bugs that must be fixed. Please read the whole thing first
> because many issues are related.
>

(elided comments are all fixed now).

>
> * Comm::Connection class is not documented. We need to be very clear
> about that class scope and purpose, IMO. Please see below for related
> problems and specific suggestions.
>

The class was not new and its semantics have not changed so I left it.
Now documented.

>
> * Comm::Connection lacks an assignment operator declaration. The current
> default/generated ones are wrong. If Vector<> is using them, we are in
> trouble.

Vector of connections is a non-comm layer artifact that comm can handle.
As non-comm it uses the ref-counting Comm::Connection::Pointer. If not
that is a bug.

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

>
> * Where do we set Comm::Connection::peer_ to something useful? (Just a
> question; I could not find any assignment but I probably missed it).
>

in src/http.cc:
   the HttpStateData _peer field is set from fwd->conn()->_peer

in src/peer_select.cc:
  peerAddFwdServer() generated a FwdServer in the possible-servers list
and cbdataReference() sets the FwdServer::_peer

  the new peerSelectDnsPaths() function clones _peer during a transform
of one FwdServer into a Comm::Connection for each of that FwdServer's IP
address link options (referenced with the FwdStateData or
TunnelStateData paths vector).

>
> * I am seriously worried about the raw paths pointer being passed to
> ConnectStateData, async callbacks, and being manipulated in several
> concurrent objects without any protection. For example, how do we
> guarantee that FwdState does not get destructed while ConnectStateData
> or the callback is alive and manipulating FwdState::paths? Paths must
> have cbdata locking or refcounting unless you can clearly proof that no
> protection is needed.
>

Okay. Which is preferred in this case where the paths is only valid as
long as FwdStateData exists? RefCounted or cbdataReference?

FWIW; I'm fairly confident (without definitive proof though) that the
FwdStateData will exist. Under the new scheme FwdStateData's existence
is not dependent on the FD remaining open and connected. That appears to
have been an artifact of the FwdStateData being in control of the FD
timout/close handlers so connect could be mid-DNS lookup when the
timeout occurs and FwdStateData died.

Under the current model;
   Timeout during DNS lookup results in FwdState receiving a shorter set
of paths out of peer selection and maybe aborting on its own steam if
there were none back.
  Socket early closure is controlled by ConnectStateData itself and
results in all caller code (not just FwdState) receiving the timeout
error after ConnectStateData has self-closed.
  dialer makes sure the object being dialed still exists, or drops the
call. Am I correct?

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.

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

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.

All for a simple ref-counter pointer singleton.

Currently paths is ONLY valid for FwdStateData and TunnelStateData
callbacks where the vector is a member field of those classes.
  They are also the only code which needs to handle multiple outbound
connection objects simultaneously.

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).

>
> * 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).

If we an find paths that can use const then I'm all for this as well.
Particularly in the areas external to comm. The paths vector result is
const already. Note the caller code receiving it uses it only to verify
it does match the internally stores vector. Then uses the non-const
internal vector for actual operations.

Just saying it's not easy in light of what will happen in part 3 of the
comm cleanups.

>
> * The following few comments are about the ConnectStateData class that
> you did not create. Normally, I do not ask for bug fixes in moved code.
> However, in your particular case, you made a private comm.cc-only class
> public and started using it in high-level code. This makes you liable
> for the now-public class. Sorry.

No prob.

>
> - ConnectStateData lacks copy constructor and assignment operator
> declarations. You do not need to implement them if they are not needed,
> but the current default/generated ones are wrong.

Ah. Yes. They need dropping.

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

>
> Moreover, the relationship between new Comm::Connection and old
> ConnectStateData is rather odd: In general terms, connection is what
> connect(2) (or accept) creates. In the current code, ConnectStateData
> uses an unconnected Connection to make a connected Connection.

It's only odd if you consider Comm::Connection to be a self-adapting
class. It's semantically similar to an FD integer value.

The comm flow:

The ListenStateData API which takes in a port and IP to listen on and
spawns Comm::Connection (incoming active) which describe an active link
[ semantic: accept() operation ]

... some mess later by higher levels in messy ways involving
ConnStateData and raw FD ...

ConnectStateData API which takes pairs of descriptor Comm::Connection
(inactive) which describe IP/port for the desired destination and
produces Comm::Connection (active outbound link)
[ semantic socket() + connect() + bind() operations ].

... some mess later involving raw FD reading and writing ...

... connection closure which receives a Comm::Connection and
de-activates it.

(ignoring higher levels and async and the 'yuck' paths where
Comm::Connection is deleted and its raw FD is passed out instead, maybe
even the raw FD closed)

>
> Furthermore, FwdState keeps an array of unconnected Connections. And
> both FwdState and ConnectStateData (and others?) use the first item of
> that array specially because it is (usually) a real connection while the
> rest are just future connection detail holders.

FwdState and TunnelState only.

ConnectStateData is the state engine for processing those active
connections and finding one that will work.

>
> Meanwhile, each Connection object has the potential of holding such
> sensitive information as descriptor and peer pointer. I suspect more
> details will be added later.
>
> 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.

  Your PeerPort class == FwdServer (generated by peer select and handled
by FwdStateData when calling ConnectStateData)

  FwdState content passed to comm_openex -> socket()

  FwdState + FD from socket() passed via old comm API to
ConnectStateData for opening the specific host/port/FD link for each of
its IP addresses.

Problem 1: host/port/FD are invalid and FD is already open and being
used by things. gah! unwind all the callbacks and try again from the top.

Alternative: moving half-way back to this old model.

  Using FwdServer as originally designed but moving the DNS lookup into
FwdState where it needs to be for the tcpOutgoingAddress and related
stuff to be known.

Results in FwdState data juggling three sets of data instead of one:
   * list of FwdServer peer options
   * array of IP addresses for the top FwdServer
   * array of Comm::Connection generated from the above IP list +
outgoing setting lookups.

we get left with forwarding then for each of the cycles down those three
data sets performing its own calls to comm_openex and comm_connect_addr
along with handling the comm failover cases, early aborts and timeouts.

Problem 2: this entire 3-data-sets handling structure and code setup
breaks scope, is tricky, is fragile, duplicated entirely by
TunnelStateData and duplicated partially by: ICAP Xaction,
HttpStateData, HttpsStateData, FtpStateData, GopherStateData.

Back to the origin:

I was of the impression that the struct peer object was squid.conf data
plus runtime extras accumulated. So would not die during the lifetime of
any given connection. We may hit problems during reconfigure, but since
it is cbdata referenced...

Are you getting it confused with ps_state, which is the temporary state
while processing peerSelect() ? That state object now has no bearing on
the forwarding process outside of generating the paths vector content.
  When the vector is generated ps_state is self-destructed and never
used again.

NP: the read/write etc handlers that operate after the connection is
made do not (yet) utilize Comm::Connection and the
ConnectStateData::EarlyAbort handler during connection setup calls the
peer API to handle a broken peer same as in the old code.

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

>
> * You have changed the semantics of the Icap::Xaction::connection
> member. It used to be set during connect and now it is only set if
> connect was successful. doneWithIo(), bypassFailure() and possibly other
> code relies on the old semantics which is no longer true.

I thought as much. The failure and bypass bits should be keying off the
COMM_* status result in the connect attempt callback now.
  I've tried to copy the previous comm action timeout and error handlers
in calling code into the right place in the unified connect handler. Did
I get it wrong for the Xaction?

The semantic being that they cannot have failed if not yet attempted.
But when comm connect returns it sends back distinct
success/fail/timeout status for caller to work with.

>
> My understanding is that you cannot set the connection member where it
> was set because the descriptor is not available at that time. The best
> solution is probably to change the connection member type from
> int/descriptor to Comm::Connection pointer (if we still have it after
> the above changes).

You understand the problem correctly.

I was floundering in the dark on that point as to what else the
connection was used for after the connection was activated. Rolling
Comm::Connection out in place of FD was to be a later stage of comm cleanup.

Yes if Xaction holds a Comm::Connection::Pointer in the connection field
the Connection details will remain alive until at least its unset. The
(connection->fd == -1) test or in the comm handler the COMM_OK status
will be usable to identify whether the connection link is active for
read/write or closed.

What I was unaware of was that the bypassFailure() or doneWithIo() may
enter without having received a connection result. That will need to be
fixed.

>
>
> * The new code below sets the connect timeout handler after a successful
> connect. That does not make sense to me.
>
> + // TODO: do we still need the timeout handler set?
> + // there was no mention of un-setting it on success.
> +
> + // TODO: service bypass status may differ from that of a
> + typedef CommCbMemFunT<Adaptation::Icap::Xaction, ...
> + AsyncCall::Pointer timeoutCall = asyncCall(93, 5, ...
>
> +
> + commSetTimeout(io.conn->fd, ...
>
> I do not think you need the above at all. The timeout handler will be
> set in updateTimeout, which will be called from scheduleWrite(). This
> logic is not very clear. If you want to improve it, you may call
> updateTimeout() instead of doing the above (it will clear the timeout
> then) but doing so is outside your work scope.

Removed.

Thanks. I was hoping that could die. Not being able to find where it
was unset left me a bit confused about what it was used for.

Build testing the changes made so far now.

Amos

>
> On 05/22/2010 12:53 AM, Amos Jeffries wrote:
>> Alex Rousskov wrote:
>>> On 05/19/2010 07:05 AM, Amos Jeffries wrote:
>>>> Henrik Nordström wrote:
>>>>> tis 2010-05-18 klockan 23:34 +0000 skrev Amos Jeffries:
>>>>>
>>>>>> I've discovered the VC connections in DNS will need a re-working to
>>>>>> handle
>>>>>> the new TCP connection setup handling. I've left that for now since it
>>>>>> appears that you are working on redesigning that area anyway. The new
>>>>>> setup
>>>>>> routines will play VERY nicely with persistent TCP links to the
>>>>>> nameservers.
>>>>> I have not started on the DNS rewrite yet.
>>>>>
>>>>>> I took some extra time last night and broke the back of the selection
>>>>>> and
>>>>>> forwarding rewrite. I'm now down to the fine detail build errors. When
>>>>>> those are done I'll push the branch to LP for you to do the DNS
>>>>>> fixes on
>>>>>> top of.
>>>>> Ok.
>>>>>
>>>> Pushed to launchpad: lp:~yadi/squid/cleanup-comm
>>> How can I review the changes as one patch without checking out your
>>> branch?
>>>
>> Oh. Attached is my latest code.
>>
>> This has now been run tested and I've been using it for personal
>> browsing for a short while now no problems.
>>
>>
>>
>>> Thank you,
>>>
>>> Alex.
>>>
>>>
>>>> This builds, but has not yet been run tested.
>>>>
>>>> What has changed:
>>>>
>>>> ConnectionDetails objects have been renamed Comm::Connection and been
>>>> extended to hold the FD and Squids' socket flags.
>>>>
>>>> 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.
>>>>
>>>> Various connection openers have been converted to use the new
>>>> ConnectStateData API and CommCalls (function based so far).
>>>>
>>>>
>>>> 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.
>>>>
>>>> ConnectStateData::connect() will go away and do some async work. Will
>>>> come back at some point by calling the handler with COMM_OK,
>>>> COMM_ERR_CONNECT, COMM_TIMEOUT and ptrs to the Comm::Connection or
>>>> vector (whichever were passed in).
>>>> On COMM_OK the Comm::Connection pointer or the first entry of the
>>>> vector will be an open conn which we can now use.
>>>> On COMM_ERR_CONNECT the vector will be empty (all tried and
>>>> discarded), the single ptr will be closed if not NULL.
>>>> On COMM_TIMEOUT their content is as per COMM_ERR_CONNECT but the vector
>>>> may have untried paths still present but closed.
>>>>
>>>> FD opening, FD problems, connection errors, timeouts, early remote
>>>> TCP_RST or NACK closure during the setup are all now wrapped out of
>>>> sight inside ConnectStateData.
>>>>
>>>> The main-level component may set FD handlers as needed for read/write
>>>> and closure of the link in their connection-done handler where the FD
>>>> first becomes visible to them.
>>>>
>>>>
>>>> Besides the testing there is some work to:
>>>> * make it obey squid.conf limits on retries and paths looked up.
>>>> * make DNS TCP links ('VC') work again.
>> The ref-counting helped a bit here not having to juggle the new/delete
>> locations. I think it goes, but have not had a conclusive test yet.
>> Still very inefficient for what it needs to do.
>>
>>>> * make the CommCalls proper AsynCalls and not function handler based.
>>>> * make Comm::Connection ref-counted so we can have them stored
>>>> in the peer details and further reduce the DNS steps.
>> done.
>>
>>>> * make ICAP do DNS lookups to set its server Comm::Connection properly.
>>>> For now it's stuck with the gethostbyname() blocking lookup.
>>>>
>>>>
>>>> Future work once this is stable is to:
>>>> a) push the IDENT, NAT, EUI and TLS operations down into the Comm layer
>>>> with simple flags for other layers to turn them on/off as desired.
>>>> b) make the general code pass Comm::Connection around so everything
>>>> like ACLs can access the client and server conn when they need to.
>>>>
>>>> Amos
>> Amos
>>
>

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