Re: [PATCH] Subscription

On 06/10/10 18:15, Alex Rousskov wrote:
> On 09/23/2010 10:05 AM, Amos Jeffries wrote:
>
>> Including the generalized Subscription mechanism used by ConnAcceptor.
>>
>> Alex: removing the XXX marked const problems and testing they will work
>> is blocked behind some build errors from the IOCB definition change. As
>> soon as I have building code that wont hide const side-effects I'll be
>> removing that muckup.
>
> OK. I could not understand whether you wanted to remove comments,
> restore const, or remove const, so I just commented on whether const is
> feasible. Either way is OK, I guess, so we should probably use const
> while we can. Not a big deal though.

Now had time to test out the const-correctness. The dialer fixes lower
down worked. Everything is now nice and cleanly const.

>
>> === added file 'src/base/Subscription.h'
>> --- src/base/Subscription.h 1970-01-01 00:00:00 +0000
>> +++ src/base/Subscription.h 2010-09-23 15:37:02 +0000
>> @@ -0,0 +1,50 @@
>> +#ifndef _SQUID_BASE_SUBSCRIPTION_H
>> +#define _SQUID_BASE_SUBSCRIPTION_H
>> +
>> +#include "base/AsyncCall.h"
>> +
>> +/**
>> + * API for classes needing to emit multiple event-driven AsyncCalls.
>> + *
>> + * The emitter class needs to accept and store a Subscription::Pointer.
>> + * The callback() function will spawn AsyncCalls to be filled out and
>> + * scheduled wth every event happening.
>> + */
>
> This is a little too restrictive/specific. Consider (lacks formatting):
>
> /** API for creating a series of AsyncCalls. A callback factory.
>
> Objects implementing this API can be given to an event "Publisher" to be
> notified of each event using a single callback-generating "subscription".

Objects implementing this API don't receive anything. They *are* the
notification. Up to "Publisher" is true.

>
> This API is necessary because the same AsyncCall callback must not be
> fired multiple times: A callback might have complex state that cannot be
> easily split into "preserve/copy" and "clear/reset" parts between the
> firings.
> */

K. Used parts of that.

>
> You may also want to add something like:
>
> \todo Add a way for Subscriber to cancel the subscription and for
> Publisher test for subscription cancellation?

This is already provided by RefCount API inherited to Subscription::Pointer.

The Subscription::Pointer can be set to NULL for cancellation/erasure,
set to point at another subscription, and tested for NULL.
The original Call pointed to is never scheduled and can be deleted
without canceling right?.
Any Calls spawned are outside the scope of the subscription itself once
they leave the callback() method.

>
>> +class Subscription: public RefCountable
>> +{
>> +public:
>> + typedef RefCount<Subscription> Pointer;
>> +
>> + /// returns a call object to be used for the next call back
>> + virtual AsyncCall::Pointer callback() = 0;
>> +// virtual AsyncCall::Pointer callback() const = 0;
>
> I think it is fine to have this method as "const": A subscription to a
> daily newspaper does not really change with every delivery. It can be
> argued that if subscription is for a limited number of deliveries, than
> it does change with every delivery, but we do not have such
> subscriptions yet. We can relax this later if needed.
>
> Please document whether callback() can return a nil pointer and, if yes,
> what would that mean to the Publisher. I suggest that it must not return
> nil.

It could be. Depends on the inheriting childs implementation.

   RefCount<AsyncCall> a;
   Subscription::Pointer b = new NilSubscription<AsyncCall>(a)
   b->callback();

NP: I'm updating CallSubscription to check and enforce non-NULL.

>> +};
>> +
>> +/**
>> + * Implements Subscription API using Call's copy constructor.
>> + *
>> + * A receiver class allocates one of these templated from the Call type
>> + * to be received (usually AsyncCallT) and passes it to the emitter
>> class
>> + * which will use it to spawn event calls.
>> + *
>> + * To be a subscriber the AsyncCall child must implement a copy
>> constructor.
>> + */
>
> Consider:
>
> /** Implements Subscription API using callback's copy constructor.
> *
> * The subscriber creates a subscription object using a specific
> * callback type as the template parameter and gives the subscription
> * to the event Publisher.
> *
> * Call_ must have a copy constructor.
> * A pointer to Call_ must be convertable to AsyncCall::Pointer
> */
>
>
>> +template<class Call_>
>> +class CallSubscription: public Subscription
>
> The class name seems wrong. All Subscription kids are "Call
> subscriptions".

There is only one kid of Subscription. The AsyncCall template. How it
callback() is implemented is a mere detail which may be changed if
AsyncCall semantics ever change.
  This is a Subscription which spawns AsyncCall.

> How about CopyingSubscription, CallCopySubscription, or
> CallbackCopier?
>
>> +{
>> +public:
>
> Consider adding here (and using below):
>
> typedef RefCount<Call_> CallPointer;
>

The typedef adds more bytes to the code than it saves. I think its clear
enough as it is in such a small class and lack of abstraction helps make
the debugger messages match the lines of code.

If the template gets more complex with more references to the pointers
it remains a possibility.

>
>> + CallSubscription(const RefCount<Call_> &aCall) : call(aCall) {};
>> +
>> +// XXX: obsolete comment?
>> + // cant be const sometimes because CommCbFunPtrCallT cant provide a
>> const overload.
>> + // CommCbFunPtrCallT lists why. boils down to Comm IO syncWithComm()
>> existence
>> + // NP: we still treat it as const though.
>> + CallSubscription(RefCount<Call_> &aCall) : call(aCall) {};
>
> Please add "explicit" to minimize silent conversion surprises.

Done.

>
> Extra semicolon after }. Above and below.
>

Fixed.

>
> Glad you found a way to keep this compact and non-intrusive. IMO, this
> can be polished and committed now if you wish. No need to wait for the
> rest of the comm-cleanup changes to settle down.

Done. Thanks for coming up with this. I can see a great future ahead for
it :)

Amos

-- 
Please be using
   Current Stable Squid 2.7.STABLE9 or 3.1.8
   Beta testers wanted for 3.2.0.2