Re: [PREVIEW] Comm::Connection async jobs

On Mon, 06 Sep 2010 08:22:24 -0600, Alex Rousskov
<rousskov_at_measurement-factory.com> wrote:
> 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.

Ah. At present just one in doAccept() covers the entire call chain.
Failures in acceptNext() by an (ab)user throw into their sync callers
exception handling and the ConnAcceptor job itself does not die. It may
later when it does its own thing, but that flows through doAccept().

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

The old code does a one-off then creates a whole new listener if a bad
connect packet came in. It works slightly better with less code if we make
it a full subscription then handle the attack cases clearly and explicitly
in the handler.

Making that change we do not actually need to get a deathCheck feedback.
FTP holds the conn which is being listened on and can close it to kill the
acceptor cleanly, the acceptor can on failure close its conn and call the
subscribed handler with an error flag. In either case the callback starting
with a closed conn aborts.

Amos
Received on Mon Sep 06 2010 - 22:07:43 MDT