Re: [PATCH] IPC subscriptions mechanism

On 08/16/2011 01:27 AM, Amos Jeffries wrote:
> 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.

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

I would try allocating the global dynamically when first needed instead
of statically. This will avoid both polluting inlines and static
initialization ordering problems.

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

Sorry, I probably missed this documentation when looking at the original
patch. The above quotes looks OK to me.

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

I do not think IPC-specific things should go into Subscription API
documentation and some [future] Subscription applications could be
specific to receiving a subset of messages, I guess.

Thank you,

Alex.
Received on Tue Aug 30 2011 - 23:19:18 MDT