Alex Rousskov wrote:
On 07/18/2010 04:52 AM, Amos Jeffries wrote:
This updated my previous submission for the comm connection opener model.
diff since last time:
- removed the multi-path pathways. They are now isolated at the upper
level. ConnOpener now handles only one Comm::Connection.
- implemented ConnOpener as an AsyncJob child.
What has changed from HEAD:
ConnectionDetails objects have been renamed Comm::Connection and been
extended to hold the FD and Squids' socket flags.
...
On COMM_ERR_CONNECT and COMM_TIMEOUT the conn will be !isOpen() or NULL
and should not be relied on.
If the connection pointer is NULL, a job opening multiple connections
would not be able to tell which connection just failed. Let's not hide
connection pointer from the initiator.
* Can Adaptation::Icap::Xaction::closeConnection set connection to NULL?
In the reuseConnection == false case, it can after the connection->close().
* Connection::close should set fd to -1 only when open (since we have to
spent time doing the check, let's use the result).
Done. Using conn->isOpen() which was already present.
* declare ConnOpener::start() as virtual, especially since you declare
other API methods virtual.
Done.
* ConnOpener::start(), doneAll(), and swanSong() should remain protected
as they are in the AsyncJob.
Done. Apart from doneAll(). It is required to be accessible to the
ConnectRetry wrapper until comm write callbacks are turned into methods.
* IMO, "********" distract and encourage others to put even more
fanciful decorations into the code.
Sigh. So pretty... Removed.
* ConnOpener::operator = should return a reference to ConnOpener, not
just ConnOpener.
Done.
* You document private and protected methods in .h files where they are
declared. Did not we agree to document private and protected methods in
.cc files, where they are defined? Or was I doing it wrong for the last
few patches?
We did. Sorry. Fixed.
* s/solo/conn/ and adjust its documentation since ConnOpener no longer
handles multiple connections?
Done.
* Move connect_timeout declaration lower, just above connstart to reduce
padding?
Done.
* s/struct _calls/struct Calls/
Done.
* Naming consistency:
s/connstart/connStart/
s/earlyabort/earlyAbort/
s/connect_timeout/connectTimeout/
s/fail_retries/failRetries/
clash with member methods. Using camelCase _ suffixed private naming
instead.
* cbdataReferenceDone() already checks for NULL pointer. You can save
two lines and some cycles if you do not do it in
Comm::Connection::~Connection().
Done.
* If you do not want Connections to be copied and assigned, make the
corresponding methods private. It would produce errors earlier and the
error messages will be a little more clear.
Done.
* No need to pre-declare "class Connection" in
src/comm/ListenStateData.h because you already included comm/forward.h.
Done.
* Do you need "Comm::" prefixes inside Comm namespace in comm/forward.h?
* Please remove the following line. It is a waste of CPU cycles, not to
mention a blunt layering violation:
memset(&calls, 0, sizeof(calls));
Done.
* Please remove these redundant and inconsistent lines in
ConnOpener::~ConnOpener():
+ solo = NULL;
+ calls.earlyabort = NULL;
+ calls.timeout = NULL;
Done.
* I do not recommend putting debugging in doneAll(). The best place to
report state is in status(), which is called automatically before and
after every job call (and can be used in the debugging statements inside
the job, of course).
Killed.
* doneAll() should call its parent, even if the current parent always
returns true.
Done.
* Should ConnOpener::swanSong() call comm_close instead of just close()?
No. Until ConnOpener() has completed successfully there is no other
valid user of the conn->fd. The only handlers to deal with are
ConnOpeners' earlyAbort and timeout handlers, which are explicitly
cancelled and removed.
comm_close does a lot of additional handler and callback state
updating over multiple callbacks. This is all unnecessary waste of
cycles on a closed or maybe half-open FD.
As you noticed the earlyAbort and timeout handler removal and
cancellation only happens if the ConnOpener knows the FD is sane enough
to do a removal before scheduling the callback. Otherwise the calls are
left untouched and swanSong cleans up.
You said I was to account for swanSong being called under any unknown
circumstances, so I've made it handle any possible state of ConnOpener
prior to destructor. The important local fields are removed in an order
to reduce the work done by callCallback() to absolute minimum in case of
more errors.
callCallback is the "normal" exit point for regular operations and
needs to cleanup with comm_remove_* and touch a lot of complex fde table
state if things have run properly. On some of the swanSong error states
(half-open FD hits Must() somewhere in INPROGRESS state) this is not
possible and WILL crash Squid.
* ConnOpener::callCallback() clears the close handler and timeout
depending on solo state, which is kind of wrong, but the best thing to
do is to remove that cleanup code from callCallback() completely because
the same code is already present in swanSong() and because it is
unrelated to calling the callback.
Ideally yes. I'm not certain enough about the state of the Job on the
second (ConnectRetry) legacy entrance to remove either at this point.
When the Write handling is able to be a method properly then the
swanSong() pre-cancellations can be removed.
For now we have the NULL checks saving cycles where possible without
adding any risky situations.
* It is better not to not assert() inside async job code. Use Must() and
let Squid kill the job instead of killing Squid. One could make an
exception for before-start() code and for the destructor, but even those
are debatable.
You mean the one in callCallback()? Removed.
* solo->fd > 0 is inconsistent with other checks. I would recommend
replacing (solo != NULL && solo->fd >= 0) with an inlined
hasConnection() or similar method to avoid these kind of problems.
Updated to conn_->isOpen() after renaming.
* The /* handle connecting to one single path */ comment is obsolete.
Updated.
* It may not be a good idea to separate calls.earlyabort and
calls.timeout existence from the existence of the corresponding Comm
close or timeout handler. Set them when setting the Comm handler. Clear
them when clearing the handler.
What do you mean? they _are_ the close and timeout handlers for the
duration of the ConnOpener activity. Set on comm_openex and removed on
ConnOpener::callCallback()
* I still think that keeping an array of unopened connections and using
the first entry as "real connection" in FwdState is wrong. However, if
you insist on doing that, let's at least hide that ugliness using a few
simple inlined methods:
- s/paths.size() > 0 && paths[0]->fd > -1/connectedToServer()/
- s/paths[0]->/serverConnection()/
The serverConnection() method should throw or assert if paths are empty.
Actually, you already have conn() method so you can just fix that: make
it return a reference to the pointer and check that paths are not empty.
this is especially important since we call conn() from outside the
FwdState object.
Fixed and changed to serverConnection(). All code pulling paths[0] for
use changed to use the wrapper.
* "if (paths[0]->fd > -1)" statements in the reforwarding code do not
check that the paths are not empty. Other FwdState code does. Should we
check here too? Or do not check in other contexts?
Same question for FwdState::serverClosed() and some other FwdState code.
Let's be safe and consistent everywhere.
Added FwdState::isServerConnectionOpen() to unify the tests.
* FwdState::unregister() used to prevent FwdState from closing the
unregistered connection. The new code does not. Was the old code buggy?
Only in Comm::Connection semantics. Added an alternative version and
deprecated the old unregister(int) with a hack to keep it going on the
old semantics.
* The following code is redundant as the compiler will do similar stuff,
just more efficiently before exiting the "conn" context:
conn = NULL; // maybe release the conn memory
Small optimization. Pinned and persistent connections can nest fairly
deeply. With a conn at each level. This releases those resources as soon
as we are surely done with them.
* Shifting the hosts array is quite expensive as a lot of refcounted
pointers need to be re-assigned. Would it be better to replace 0 with
connIdx and increment that index instead of shifting?
For performance yes. I'll look at implementing that boost later.
* Empty comment near:
+ char nextHop[256]; //
Removed.
* In the description below,
s/possible paths .../connections to open, in order, until successful/
because it is not clear from the context what "path" is.
+ /** possible paths which may be tried (in sequence stored) */
+ Comm::Paths paths;
Done.
And why is it called "paths"? Each paths element (i.e., a path) only
contains one point (the next hop). A path is a sequence of points.
Should we call the "paths" member "hops", "peers", or "servers"?
Sequence of two location data points are stored in each conn:
(local,remote)
The paths vector<> holds a series of such.
Though I agree paths is not a good name, and I am leaning towards
remoteDestinations or somesuch now.
* ftp.cc contains duplicate FtpStateData::ftpPasvCallback creation code
that leads to AsyncJob::AsyncStart(cs). Please move it into a new
startPassiveFtp() method or some such.
Disagree. The conn details are not the same. Only the name of the
callback and the fact its now starting a ConnOpener. Which means a new
file-global function is needed just for four lines of code.
Maybe when FtpState is cleaned up further.
* FwdState::complete may shift paths (via reforward). If that happens,
the following will close a different/new connection or assert/throw (or
even segfault in the current patch):
fwd->complete();
- comm_close(fd);
+ fwd->conn()->close();
I gopher.cc? Good catch. Rolling that change back.
Overall, exposing connection pointer to Servers seems wrong, especially
because the pointer may change from one call to another. A Server that
needs that pointer, should store it at initialization time, just like it
stores fd in the current code.
You have "TODO: store Comm::Connection instead of FD". This is an XXX
and not just a TODO because of the above concerns.
This auditing task is scoped to do the connection setup and get
split-stack IPv6 a chance of going operational. Which means as little
upper layer change in the Servers as we can cope with.
The following steps are to cleanup ListenStateData and CommCalls a bit
more then to roll conn across all the objects which as FD between
themselves.
Which reminds me:
Is there any reason why the CommCalls have unique params classes and
dialers for CommTimeoutCb* and CommCloseCb* ? I'd kind of like to roll
them and CommConnectCb* into passing CommCommonCb* if thats possible,
and cut ~25% of the CommCalls code on the next cleanup step.
* Will we start leaking peers after the change below?
- if (fwd->servers)
- _peer = fwd->servers->_peer; /* might be NULL */
+ if (fwd->conn() != NULL)
+ _peer = cbdataReference(fwd->conn()->getPeer()); /* might be
NULL */
I cannot find the corresponding cbdataReferenceDone added somewhere.
Haven't seen any. Though it may be happening. Added to the matching
destructor.
* Creating a temporary refocunted pointer to a local variable is a bad
idea, no matter what Ident::Start does. The object will be deallocated
twice: once at the end of the if-statement, and once when the refcounter
reaches zero:
+ Comm::Connection cc; // IDENT will clone it's own copy
+ Comm::ConnectionPointer ccp = &cc;
Okay. Made it dynamic.
* Declare Comm::ConnectionPointer p inside the for-loop in
peerSelectDnsResults() if that variable is not used outside the loop.
Done.
That is all I had time for, sorry.
Alex.
All changed. Running my checks again now before commit and re-submit.
Amos
--
Please be using
Current Stable Squid 2.7.STABLE9 or 3.1.5
