Re: [PREVIEW] Comm::Connection async jobs

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.

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

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

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

* ConnAcceptor is missing a class description.

* Please mark virtual methods as such, even in kids. For example,
ConnAcceptor::~ConnAcceptor() is virtual. It is not required for the
code to work, but it brings our attention that the method is a part of
some API.

* Please make ConnAcceptor::notify() protected if possible.

* Consider adding a "const char *reason" parameter to unsubscribe, for
debugging.

* Never call swanSong() directly, especially from the destructor. Even
we disregard the API, calling a virtual function from a destructor is a
bad idea. As far as I can see, you may not need a destructor after
switching to the Subscription API.

* The documentation for ConnOpener constructor is wrong because the
constructor does not open anything. Please remove. We usually do not
document constructors unless they do something special rather than just
constructing an object (same for other standard methods).

* The documentation for non-existent ConnOpener copy-constructor and
assignment operator needs polishing. The former is not an operator. Both
essentially create copies, yet the docs differ. Perhaps you can replace
each with the following?
     /// Undefined because two openers cannot share a connection

* Please remove tryConnecting() if unused.

Thank you,

Alex.
Received on Sun Sep 05 2010 - 19:01:22 MDT