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
