[asterisk-dev] [Code Review] 3099: CDRs and Stasis take 2: Synchronize publication of application CDR messages to the CDR engine subscription

[Matt Jordan]

> On Jan. 9, 2014, 4:12 p.m., David Lee wrote:
> > /branches/12/main/stasis.c, lines 634-647
> > <https://reviewboard.asterisk.org/r/3099/diff/2/?file=51275#file51275line634>
> >
> >     I'm curious why you didn't just dispatch_message(sub, message, sync_sub == sub);
> >     
> >     As it's written, sync_sub will receive the message, even if it's not subscribed to the topic in question.
> >     
> >     But since we're just talking about the behavior of an error condition, I'm fine either way.

My initial thought was to push the message out asynchronously to the non-specified subscribers, then block the callback on the synchronous subscriber. That's probably overkill, however, since no one will be able to queue up a message to the asynchronous subscribers while we do this: we've locked the topic. Fixed.

As an aside, since I'm currently using this by going through the message router, that actually can't happen - the topic is pulled from the message router's reference to the topic, then passed into here. So you're guaranteed by that route that the topic and subscriber belong together. That doesn't mean diddly for the general case however.


- Matt


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


On Jan. 9, 2014, 9:16 a.m., Matt Jordan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3099/
> -----------------------------------------------------------
> 
> (Updated Jan. 9, 2014, 9:16 a.m.)
> 
> 
> Review request for Asterisk Developers and David Lee.
> 
> 
> Bugs: ASTERISK-22884
>     https://issues.asterisk.org/jira/browse/ASTERISK-22884
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> In https://reviewboard.asterisk.org/r/3057/, applications and functions that manipulate CDRs were made to interact over Stasis. This was done to synchronize manipulations of CDRs from the dialplan with the updates the engine itself receives over the message bus.
> 
> This change rested on a faulty premise: that messages published to the CDR topic or to a topic that forwards to the CDR topic are synchronized with the messages handled by the CDR topic subscription in the CDR engine. This is not the case. There is no ordering guaranteed for two messages published to the same topic; ordering is only guaranteed if a message is published to the same subscriber.
> 
> Note that the patch actually still fixed a bunch of test failures by virtue of publishing the messages in the first place to the channel topic; however, the hangup handler tests - which use the CDR function to read a value from the engine - did still fail after this patch. Reading data requires an explicit synchronization with the engine.
> 
> In order to fix this problem, this patch does the following:
> (1) It provides a new way to publish a message. It is now possible to publish a message and specify which stasis_subscription you want to synchronize on. A message published in such a fashion is still published to all subscribers of the topic (which is needed; you don't know who all wants your message), but the call will block until the specified subscriber handles the message.
> (2) It updates the CDR engine to exposed its message router (which contains the subscription). CDR related applications now synchronize on the message router when publishing information. As a result, the CDR topic/router now persist across CDR engine disablings; the only thing that is removed when the engine is disabled are the forwarding subscriptions. This keeps a lot of quirky lifetime issues from becoming serious problems, while still maintaining performance if the CDR engine is disabled.
> 
> 
> Diffs
> -----
> 
>   /branches/12/tests/test_stasis.c 404591 
>   /branches/12/main/stasis_message_router.c 404591 
>   /branches/12/main/stasis.c 404591 
>   /branches/12/main/cdr.c 404591 
>   /branches/12/include/asterisk/stasis_message_router.h 404591 
>   /branches/12/include/asterisk/stasis.h 404591 
>   /branches/12/include/asterisk/cdr.h 404591 
>   /branches/12/funcs/func_cdr.c 404591 
>   /branches/12/apps/app_forkcdr.c 404591 
>   /branches/12/apps/app_cdr.c 404591 
> 
> Diff: https://reviewboard.asterisk.org/r/3099/diff/
> 
> 
> Testing
> -------
> 
> Unit test for publishing synchronously was added. It passes.
> 
> All stasis and CDR unit tests pass.
> 
> All CDR and hangup handler test in the test suite now pass. Previously, the hangup handler tests would fail due to getting wrong data back from the CDR engine when reading a value using the CDR function.
> 
> 
> Thanks,
> 
> Matt Jordan
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140109/f1bfb156/attachment-0001.html>