Re: [PREVIEW] Comm::Connection async jobs

On 09/06/2010 07:08 AM, Amos Jeffries wrote:
> On 06/09/10 07:01, Alex Rousskov wrote:
>>
>> * 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.

I meant "from AsyncCalls", sorry.

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

I think that is a good start. However, you should not spray the
ConnAcceptor code with try/catch if all you want is to kill Squid. Just
implement the callException() method and call fatal there. There should
be a few examples in the code.

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

If FTP is using ConnAcceptor, it probably should be notified when the
acceptor quits so that you can terminate the FTP transaction. Otherwise,
it may get stuck for a while. The simplest way to do that would probably
be to allow the initiator to set a deathCallback member of the acceptor.
If deathCallback is set, call it in swanSong.

BTW, does FTP need a subscription mechanism or a one-time notification?
If one time, you can either add a oneTime boolean member to the
Subscription API or let FTP customize the Subscription class. The choice
depends on whether you think other classes would need a one-time
subscription feature.

Eventually, I think we will polish/standardize the deathCallback API but
that can wait until client-side polishing or another big project that
needs it.

HTH,

Alex.
Received on Mon Sep 06 2010 - 14:22:42 MDT