Re: [PATCH] IPC subscriptions mechanism

On 16/08/11 10:02, Alex Rousskov wrote:
> On 08/13/2011 06:47 AM, Amos Jeffries wrote:
>> On 13/08/11 05:23, Amos Jeffries wrote:
>>> This adds support for components to register to receive AsyncCalls when
>>> IPC messages are received.
>>>
>>> The subscription is linked to a message type. With currently one
>>> registered recipient Job per type.
>>>
>>
>> Updated version. Includes a new file ipc/Messages.cc to define the
>> subscriptions registry global.
>>
>> Now tested and works.
>
> Can you hide Ipc::MessageSubscriptions from subscribers? It seems like
> that variable does not need to be globally visible, and there is no need
> to inline registration management.

Not in its current form. It is used by Ipc::Coordinator internals. The
inlining prevents initialization issues with library internal globals.

Tried making them members of Ipc::Coordinator to get around both those
problems. Should work but I'm hitting an assert now on the
Ipc::Coordinator::receive dynamic cast.

>
> Consider relaxing the RegisterListenFor() check to allow repeated
> registrations if callSub has not changed. After all, you do allow
> repeated unregistrations.

Okay. Allowing avoidance of the assert. But documenting as unique
registrations required, so we don't get any funny attempts at abuse.

>
> It may be worth documenting that registration API is for listening to
> _all_ messages of a certain type, and is not suitable for catching an
> individual response to a specific IPC request.

AKA. "subscribe a callback to happen whenever a certain message class
arrives"
  ... no mention of termination (until un-registered)
  ... or numbers (indeterminate, even if unregistered on first receipt).

ClearListen for states "already scheduled events may exist and will
happen". Plural on the events queued at de-registration time.

Seems pretty unambiguous that this is starting and stopping a stream of
"things", rather than discrete reads. This is true of all subscriptions.
Even the FTP data listener is written to wade through N data channel
attacks trying to find the one which was the actual server.

How else could this be worded?

... and I think that wording should go into the Subscription API
template documentation. Since its always true for Subscriptions.

> The caller is supposed to
> write a second layer mapping/registration routine that will map
> individual responses to the code/state waiting for them.
>

see ipc/Messages.h
"
  * Subscribers must create a Dialer and a AsyncCall to receive
TypedMsgHdr parameters
  * then generate a new CallSubscription<Dialer>(AsyncCall).
  * then use RegisterListenFor(message type, subscription) to start
receiving IPC messages
"

There is no requirement on what the receiver may do when getting a
TypedMsgHdr. Might translate to some other type. Might use TypedMsgHdr
directly. Might do nothing. These patches are not changing the existing
limits on IPC messages.

Fixed spelling and changed that to say "const TypedMsgHdr" though, so
its clear they must not write to it.

Amos

-- 
Please be using
   Current Stable Squid 2.7.STABLE9 or 3.1.14
   Beta testers wanted for 3.2.0.10