[asterisk-dev] [Code Review] 2738: Stasis - tweak caching topics to fix CEL tests

[Matt Jordan]

> On Aug. 3, 2013, 3:29 a.m., Matt Jordan wrote:
> > /trunk/main/stasis_cache_pattern.c, lines 169-185
> > <https://reviewboard.asterisk.org/r/2738/diff/1/?file=43757#file43757line169>
> >
> >     So earlier tonight I was getting a lot of crashes when dummy channels were disposed of. They were attempting to dispose of their caching topic only to find that that topic was already disposed of.
> >     
> >     After a lot of ref count debugging, the best I could figure was that the reference to the caching topic held by one of the subscriptions was causing the caching topic destructor itself to fire when the subscription was fired. This made some "sense", as the subscription also holds a reference to the topic - but since the channel also holds a reference, I'm not sure how this is expected to work.
> >     
> >     Then, with this patch, it all magically started working again. I can only surmise that the act of unsubscribing from the forwarding topics before unsubscribing from the caching topic resolves the issue.
> >     
> >     The reference owning semantics between subscriptions and topics is *incredibly* complex. I'm not sure if there's a solution to this problem, but it feels like subscriptions going away should never impact the topic that was subscribed to so long as something still holds a reference to the topic.
> 
> David Lee wrote:
>     Agreed, but that's an underlying problem with subscription/topic
>     references; not something introduced with this patch. This patch
>     'fixes the glitch', but doesn't address the underlying problem you
>     found.
>     
>     Yes, the reference counting is complex. Unfortunately, we have cyclic
>     references betweeen subscriptions and topics, and that's something
>     that really befuddles a reference counting system. Add to that lots of
>     temporary references that happen while dispatching the unsubscribe
>     messages, and it gets really weird fast.
>     
>     I'm sure there's something we can do to improve the situation, but
>     that's outside the scope of this patch.

Agreed. I shouldn't have opened it as an "issue".

Since we are fixing a ref counting semantics issue through this patch, however, do you mind adding a more verbose description of what holds references to what in some portion of the Stasis code?

The only reference to the reference issue (hah, English language is funny) that I found is:

{quote}
 * In order to stop receiving messages, call stasis_unsubscribe() with your \ref
 * stasis_subscription. Due to cyclic references, the \ref
 * stasis_subscription will not be freed until after it has been unsubscribed,
 * and all other ao2_ref()'s have been cleaned up.
{quote}

That's good to know from an API usage standpoint, but the complexity involved with maintaining this might warrant a bit more of an explanation elsewhere.


- Matt


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


On Aug. 2, 2013, 10:56 p.m., David Lee wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2738/
> -----------------------------------------------------------
> 
> (Updated Aug. 2, 2013, 10:56 p.m.)
> 
> 
> Review request for Asterisk Developers and kmoore.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> The Stasis changes in r395954 had an unanticipated side effect: messages
> published directly to an _all topic does not get forwarded to the
> corresponding caching topic.
> 
> This patch fixes that by changing how caching topics forward messages,
> and how the caching pattern forwards are setup.
> 
> For the caching pattern, the all_topic is forwarded to the
> all_topic_cached. This forwards messages published directly to the
> all_topic to all_topic_cached.
> 
> In order to avoid duplicate messages on all_topic_cached, caching topics
> were changed to no longer forward uncached messages. Subscribers to an
> individual caching topic should only expect to receive cache updates,
> and subscription change messages. Since individual caching topics are
> new, this shouldn't be a problem.
> 
> There are a few minor changes to the pre-cache split behavior.
> 
>  * For topics changed to use the caching pattern, the all_topic_cached
>    will forward snapshots in addition to cache updates. Since
>    subscribers by design ignore unexpected messages, this should be
>    fine.
> 
>  * Caching topics that don't use the caching pattern no longer forward
>    non-cache updates. This makes no difference for the current caching
>    topics.
> 
>    * mwi_topic_cached, channel_by_name_topic and
>      presence_state_topic_cached have no subscribers
> 
>    * device_state_topic_cached's only subscriber only processes cache
>      udpates
> 
> 
> Diffs
> -----
> 
>   /trunk/include/asterisk/stasis.h 396153 
>   /trunk/main/stasis_cache.c 396153 
>   /trunk/main/stasis_cache_pattern.c 396153 
>   /trunk/tests/test_stasis.c 396153 
> 
> Diff: https://reviewboard.asterisk.org/r/2738/diff/
> 
> 
> Testing
> -------
> 
> Ran unit tests several times.
> 
> 
> Thanks,
> 
> David Lee
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130805/03d727e9/attachment.htm>