[asterisk-dev] [Code Review]: Initial stasis-core message bus

David Lee reviewboard at asterisk.org
Fri Mar 1 10:03:04 CST 2013 


> On Feb. 21, 2013, 4:32 p.m., Matt Jordan wrote:
> > /trunk/main/stasis_cache.c, lines 335-352
> > <https://reviewboard.asterisk.org/r/2339/diff/1/?file=33518#file33518line335>
> >
> >     I might be incorrect in this, but there's a subtle race condition here that may confuse subscribers.
> >     
> >     Say we have the following:
> >      channel-update-1 (updates cache)
> >      some-other-event
> >      channel-update-2 (updates cache)
> >     
> >     Before channel-update-1 is published, it updates the cache, then is published. The act of publishing does not mean it is immediately serviced by the various entities; rather, it is queued up for delivery to their individual task processors. Immediately behind it, some-other-event is published, then a second channel update that also updates the cache.
> >     
> >     A subscriber receives notification of a cache update (1), then gets the some-other-event message. As part of that handling, it goes and retrieves the channel state from the cache.
> >     
> >     Unfortunately, channel-update-2 already updated the cache with its state, even though we haven't seen the event yet. And so we get future state with a prior event.
> >     
> >     This is especially dangerous for events that remove items from the cache - by the time something services the request, the item is gone from the cache and they can no longer request it.
> >     
> >     An alternative to this would be to update the cache after all subscribers have received the update; I'm not sure how feasible this is however.
> 
> David Lee wrote:
>     Delaying the cache update would be even more confusing. Querying the cache would return old data until some slow subscriber somewhere is finished reading it? Doesn't sound pleasant.
>     
>     I'd have to think about it more, but I believe that reading objects from the cache and receiving the events from the cache topic should be an either-or proposition. That would be the only way to make sure that you have a consistent view of the channel.
>     
>     I'll have to give this more thought.
> 
> Matt Jordan wrote:
>     I think we're going to find that as a Stasis Subscriber, when I receive an update in state from some object I need to have some way to get the previous state.
>     
>     For example, take the case of CDRs (ew). Say we have the following events:
>     
>     * Channel A Dial Begin
>     * Channel A Ringing
>     * Channel A Dial End (Busy)
>     * Channel A Hangup
>     
>     As the CDR subscriber, when I get the Channel A Ringing event, I may want to go out to the cache to get properties of the channel. If, however, the Channel A Hangup event was already processed, then Channel A doesn't exist in the cache. True, I'll still get the events - but I can no longer get information about the channel out of the cache, and if I need to store properties of the channel, I'm going to have to cache them locally.
>     
>     In the case of CDRs, this means I'm going to need to cache channel state and bridge state. If I'm doing that in a subscriber module, then what purpose does the Stasis cache serve?

> I think we're going to find that as a Stasis Subscriber, when I receive an update in state from some object I need to have some way to get the previous state.

The cache does this already with the stasis_cache_update message.

> when I get the Channel A Ringing event, I may want to go out to the cache to get properties of the channel.

Wouldn't those event have channel snapshots with them?

> If I'm doing that in a subscriber module, then what purpose does the Stasis cache serve?

The primary purpose of the cache_topic is to provide cache_update messages when snapshot messages are sent. I view the ability to retrieve the most recent state directly from the cache as a nice side effect of it. But the event subscription and the direct cache retrieval, by nature, cannot be consistent with each other (unless we build a _really_ complicated cache), for the reasons you point out. If we think that will be a problem, when we should make the stasis_cache_get function private.

We could extract the underlying cache and make that available, so that subscribers can more easily build their own caches that are consistent with the messages that they've processed. That, however, feels wrong, too.


- David


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/2339/#review7909
-----------------------------------------------------------


On Feb. 18, 2013, 4:32 p.m., David Lee wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2339/
> -----------------------------------------------------------
> 
> (Updated Feb. 18, 2013, 4:32 p.m.)
> 
> 
> Review request for Asterisk Developers, Matt Jordan and kmoore.
> 
> 
> Summary
> -------
> 
> This patch adds a new message bus API to Asterisk.
> 
> For the initial use of this bus, I took some work kmoore did creating
> channel snapshots. So rather than create AMI events directly in the
> channel code, this patch generates Stasis events, which manager.c uses
> to then publish the AMI event.
> 
> This message bus provides a generic publish/subscribe mechanism within
> Asterisk. This message bus is:
> 
>  - Loosely coupled; new message types can be added in seperate modules.
>  - Easy to use; publishing and subscribing are straightforward
>    operations.
>  - Consistent memory management; all message bus objects are AO2
>    managed objects, using ao2_ref() and ao2_cleanup() to manage the
>    reference counting.
> 
> In addition to basic publish/subscribe, the patch also provides a
> mechanisms for message forwarding, and for message caching.
> 
> 
> This addresses bug ASTERISK-20959.
>     https://issues.asterisk.org/jira/browse/ASTERISK-20959
> 
> 
> Diffs
> -----
> 
>   /trunk/include/asterisk/channel.h 381699 
>   /trunk/include/asterisk/channel_internal.h 381699 
>   /trunk/include/asterisk/stasis.h PRE-CREATION 
>   /trunk/main/asterisk.c 381699 
>   /trunk/main/channel.c 381699 
>   /trunk/main/channel_internal_api.c 381699 
>   /trunk/main/manager.c 381699 
>   /trunk/main/pbx.c 381699 
>   /trunk/main/stasis.c PRE-CREATION 
>   /trunk/main/stasis_cache.c PRE-CREATION 
>   /trunk/main/stasis_message.c PRE-CREATION 
>   /trunk/tests/test_stasis.c PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/2339/diff
> 
> 
> Testing
> -------
> 
> Unit tests; manually verified AMI events for a few calls.
> 
> 
> Thanks,
> 
> David
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130301/78190b67/attachment-0001.htm>

More information about the asterisk-dev mailing list