On 06/09/10 07:01, Alex Rousskov wrote:
On 09/05/2010 05:12 AM, Amos Jeffries wrote:
To get around some layering violations between client and server side
I've found it easier to continue the rollout of Comm::Connection through
client-side that to work up a working hack.
The attached patch contains only the src/comm/ API pieces for review so
we can ensure they are done properly before much more testing and work
is done to make use of them.
Comm::Connection - data object. stores the FD, local IP+port, remote
IP+port, and comm flags relevant to the socket connection.
Comm::ConnAcceptor - was ListenStateData, but now converted to AsyncJob.
Takes a connection to listen on and emits active Comm::Connection
objects for new client connections.
Comm::ConnConnector - take a closed template Comm::Connection and make
it an active connection.
* s/ConnAcceptor::subscribe(call)/ConnAcceptor::callWhenReady(call)/ or
similar to emphasize that this is not a subscription mechanism but a
one-time arrangement. However, the subscription suggestion below makes
this change unnecessary.
Yes, it will die as soon as I can get the subscribe accepting pre-made
AsyncCall objects.
* I understand what you are doing with ConnAcceptor::subscribe() but I
think we should generalize subscription-friendly callbacks rather than
dissecting AsyncCall and trying to reconstruct it in ConnAcceptor (and,
later, in other classes). It is not going to be much more complex than
what you do now, but it will be reusable.
Specifically, I suggest adding base/Subscription.h with the following
API declaration (add refcounted Pointer type, and we can add more bells
and whistles later):
/// allows to call back the subscriber many times
class Subscription: public RefCountable {
public:
/// returns a call object to be used for the next call back
virtual AsyncCall::Pointer callback() const = 0;
};
And the following template to create actual subscription objects from
Dialers.
/// implements Subscription API using Call's copy constructor
template <class Call>
class SubscriptionViaCopyT: public Subscription {
public:
CallSubscriptionT(const Call::Pointer &aCall): call(aCall) {}
virtual AsyncCall::Pointer callback() const { return new Call(call); }
private:
Call::Pointer call; ///< gets copied to create callback calls
};
The above will probably need some polishing to get it to work, but the
idea should be clear.
You will need to disable copying of AsyncCall objects (via the usual
undefined and private copy constructor trick -- I should have done that
long time ago) and then add copy constructors to a couple of AsyncCall
kids that are already used for subscriptions in your patch.
Your ConnAcceptor will have a single
void ConnAcceptor::subscribe(const Subscription::Pointer &aSub)
method used for all subscriptions. ConnAcceptor will use sub->callback()
to get a call object for the next notification.
Nice. Will do.
* You have several Musts in Comm::ConnAcceptor. If any of them throws,
the job will quit but Squid will keep running. Same for any indirect
exceptions the acceptor code might trigger. Are you sure no other job or
module needs to be notified of the fact that there is no acceptor any
more? Usually, the initiator cares if its initiatee dies...
Hmm, yes. What should we do if squid has a massive failure on a
listening http_port?
For now I'm wrapping the accept() sequence in a try-catch which calls
fatal() if anything throws. Any better ideas welcome.
The initiators using ConnAcceptor in the current 3.HEAD are
http_port_list, http_port_list and FTP. I can see icp_port/htcp_port
being converted to use it the same way as http_port.
FTP used to d its listening a bit strange with several hacks. I'm in
process of simplifying that.
* We cannnot simply dereference raw pointers to jobs because a job may
quit and free the job object. Consider using AsyncJob::Pointer instead
and do check that the code does not dereference potentially invalid
pointers. Any public job member (data or method) is a suspect.
For example, how does Comm::AcceptLimiter::kick() know that
deferred.shift() does not point to a dead job? A Vector<> of
ConnAcceptor pointers is one example but there may be others.
Understood. These two have a rather special relationship.
ConnAcceptor registers itself into AcceptLimiter at which point its
isLimited gets incremented, once for every limiting record.
I've added the missing bits for ConnAcceptor::swanSong to safely and
quickly clear any entries it had remaining in the limiter.
* Please make ConnAcceptor::notify() protected if possible.
Noted. Unfortunately there is a bit of abuse still to be cleaned up.
* Consider adding a "const char *reason" parameter to unsubscribe, for
debugging.
Okay I'll consider.
Amos
--
Please be using
Current Stable Squid 2.7.STABLE9 or 3.1.8
Beta testers wanted for 3.2.0.2
