Re: [PATCH] Subscription

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.

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

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

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?

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

> +};
> +
> +/**
> + * 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". How about CopyingSubscription, CallCopySubscription, or
CallbackCopier?

> +{
> +public:

Consider adding here (and using below):

     typedef RefCount<Call_> CallPointer;

> + 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) {};

Yes, the constructor parameter can and should be const. You are just
copying pointers here.

Please add "explicit" to minimize silent conversion surprises.

Extra semicolon after }. Above and below.

> + virtual AsyncCall::Pointer callback() { return new Call_(call); };
> +// virtual AsyncCall::Pointer callback() const { return new Call_(call); };

This particular implementation does not change CallSubscription so it
can be constant (must be consistent with API, of course).

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.

Thank you,

Alex.
Received on Wed Oct 06 2010 - 05:15:16 MDT