[asterisk-dev] [Code Review] 3710: ARI: Subscribe to both halves of a Local channel pair when originating a Local channel to a Stasis application; clean up subscription leak

[Matt Jordan]

> On July 3, 2014, 4:12 p.m., Joshua Colp wrote:
> > /branches/12/res/ari/resource_channels.c, line 882
> > <https://reviewboard.asterisk.org/r/3710/diff/1/?file=62248#file62248line882>
> >
> >     Holding the channel lock while subscribing is hazardous. The stasis_app_subscribe will internally call ast_channel_get_by_name which locks the the individual channels during comparison. If two (or more) originates happen at the same time it is possible for one to hold a channel lock and the other to block waiting to acquire it, while the first one blocks waiting to find the channel... and yeah, no good.
> 
> Matt Jordan wrote:
>     This was not easy to track down.
>     
>     (1) stasis_app_subscribe calls app_handle_subscriptions
>     (2) app_handle_subscriptions calls app_event_source_find
>         (a) app_event_source_find traverses a list and returns back an application event source handler pointer - in this case the channel one.
>         (b) Since we are subscribing, our handler passed to app_handle_subscriptions will be app_subscribe.
>     (3) app_handle_subscriptions then calls app_subscribe (our handler). 
>     (4) This will call event_source->find, which since we have the channels event source, will call ast_channel_get_by_name
>     
>     :-(
>     
>     However, this is all unnecessary. When we originate a channel, we have:
>     (1) The application
>     (2) The channel
>     
>     We should just call app_subscribe_channel, which avoids the double look up of the channel.
>

Of course, that is in an "internal" API. Not that it has to be internal, it just is currently.

Really, the whole 8 layers of indirection is fine when you're starting off with a URI: we just aren't here. We already have the application and the channel. I'm going to look at making that call public and see what, if any, problems that might cause.


- Matt


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


On July 3, 2014, 3:50 p.m., Matt Jordan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3710/
> -----------------------------------------------------------
> 
> (Updated July 3, 2014, 3:50 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-23939
>     https://issues.asterisk.org/jira/browse/ASTERISK-23939
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This patch fixes two bugs:
> 
> (1) When originating a channel into a Stasis application, we already create a subscription for the channel that is going into our Stasis app. Unfortunately, when you create a Local channel and pass it off to a Stasis app, you really aren't creating just one channel: you're creating two. This patch snags the second half of the Local channel pair (assuming it is a Local channel pair, but luckily core_local is kind about such assumptions) and subscribes to it as well.
> 
> (2) Subscriptions are a bit sticky right now. If a subscription is made, the 'interest' count gets bumped on the Stasis subscription - but unless something explicitly unsubscribes the channel, said subscription sticks around. This is not much of a problem is a user is creating the subscription - if they made it, they must want it. However, when we are creating implicit subscriptions, we need to make sure something clears them out. This patch takes a pessimistic approach: it watches the cache updates coming from Stasis and, if we notice that the cache just cleared out an object, we delete our subscription object. This keeps our ao2 container of Stasis forwards in an application from growing out of hand; it also is a bit more forgiving for end users who may not realize they were supposed to unsubscribe from that channel that just hung up.
> 
> 
> Diffs
> -----
> 
>   /branches/12/res/stasis/app.c 417955 
>   /branches/12/res/ari/resource_channels.c 417955 
> 
> Diff: https://reviewboard.asterisk.org/r/3710/diff/
> 
> 
> Testing
> -------
> 
> The channels originate test (which makes 25 Local channels) now gets 25 channels in its Stasis application, but gets 50 destruction messages. Inspection of the log file shows that it also gets the dialplan execution messages for the Local channel halves off executing dialplan.
> 
> 
> Thanks,
> 
> Matt Jordan
> 
>

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