Re: [PREVIEW] Comm::Connection async jobs

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