<p>Joshua Colp <strong>merged</strong> this change.</p><p><a href="https://gerrit.asterisk.org/9297">View Change</a></p><div style="white-space:pre-wrap">Approvals:
Kevin Harwell: Looks good to me, but someone else must approve
Benjamin Keith Ford: Looks good to me, but someone else must approve
Richard Mudgett: Looks good to me, approved
Joshua Colp: Approved for Submit
</div><pre style="font-family: monospace,monospace; white-space: pre-wrap;">res_stasis: Fix stale data in ARI bridges<br><br>Fixed an issue that resulted in "Allocation failed" each time an ARI<br>request was made to start playing MOH on a bridge.<br><br>In bridge_moh_create() we were attaching the after bridge callbacks to<br>chan which is the ;1 channel of the unreal channel pair. We should have<br>attached them to the ;2 channel which is pushed into the bridge by<br>ast_unreal_channel_push_to_bridge(). The callbacks are called when the<br>specific channel leaves the bridging system. Since the ;1 channel is<br>never put into a bridge the callbacks never get called. The callbacks<br>then never remove the moh_wrapper from the app_bridges_moh container. As<br>a result we cannot find the channel associated with the wrapper to start<br>MOH because it has hungup. This is the reason causing the reported issue.<br><br>* Rather than using after bridge callbacks to cleanup, we now have<br>moh_channel_thread() doing the cleanup when the channel hangs up.<br><br>* Fixed moh_channel_thread() accumulating control frames on the stasis<br>bridge MOH channel until MOH is stopped. Control frames are no longer<br>accumulated while MOH is playing.<br><br>* Fixed channel ref counting issue. stasis_app_bridge_moh_channel() may<br>or may not return a channel ref. As a result ast_ari_bridges_start_moh()<br>wouldn't know it may have a channel ref to release.<br>stasis_app_bridge_moh_channel() will now return a ref with the channel it<br>returns.<br><br>* Eliminated RAII_VAR in bridge_moh_create().<br><br>ASTERISK-26094 #close<br><br>Change-Id: Ibff479e167b3320c68aaabfada7e1d0ef7bd548c<br>---<br>M res/ari/resource_bridges.c<br>M res/res_stasis.c<br>2 files changed, 37 insertions(+), 45 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><span>diff --git a/res/ari/resource_bridges.c b/res/ari/resource_bridges.c</span><br><span>index 019cdea..ab65c6f 100644</span><br><span>--- a/res/ari/resource_bridges.c</span><br><span>+++ b/res/ari/resource_bridges.c</span><br><span>@@ -821,6 +821,7 @@</span><br><span> }</span><br><span> </span><br><span> ast_moh_start(moh_channel, moh_class, NULL);</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_channel_cleanup(moh_channel);</span><br><span> </span><br><span> ast_ari_response_no_content(response);</span><br><span> </span><br><span>diff --git a/res/res_stasis.c b/res/res_stasis.c</span><br><span>index a60ec5f..aafc105 100644</span><br><span>--- a/res/res_stasis.c</span><br><span>+++ b/res/res_stasis.c</span><br><span>@@ -472,29 +472,6 @@</span><br><span> return cmp;</span><br><span> }</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">-/*! Removes the bridge to music on hold channel link */</span><br><span style="color: hsl(0, 100%, 40%);">-static void remove_bridge_moh(char *bridge_id)</span><br><span style="color: hsl(0, 100%, 40%);">-{</span><br><span style="color: hsl(0, 100%, 40%);">- ao2_find(app_bridges_moh, bridge_id, OBJ_SEARCH_KEY | OBJ_UNLINK | OBJ_NODATA);</span><br><span style="color: hsl(0, 100%, 40%);">- ast_free(bridge_id);</span><br><span style="color: hsl(0, 100%, 40%);">-}</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span style="color: hsl(0, 100%, 40%);">-/*! After bridge failure callback for moh channels */</span><br><span style="color: hsl(0, 100%, 40%);">-static void moh_after_bridge_cb_failed(enum ast_bridge_after_cb_reason reason, void *data)</span><br><span style="color: hsl(0, 100%, 40%);">-{</span><br><span style="color: hsl(0, 100%, 40%);">- char *bridge_id = data;</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span style="color: hsl(0, 100%, 40%);">- remove_bridge_moh(bridge_id);</span><br><span style="color: hsl(0, 100%, 40%);">-}</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span style="color: hsl(0, 100%, 40%);">-/*! After bridge callback for moh channels */</span><br><span style="color: hsl(0, 100%, 40%);">-static void moh_after_bridge_cb(struct ast_channel *chan, void *data)</span><br><span style="color: hsl(0, 100%, 40%);">-{</span><br><span style="color: hsl(0, 100%, 40%);">- char *bridge_id = data;</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span style="color: hsl(0, 100%, 40%);">- remove_bridge_moh(bridge_id);</span><br><span style="color: hsl(0, 100%, 40%);">-}</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span> /*! Request a bridge MOH channel */</span><br><span> static struct ast_channel *prepare_bridge_moh_channel(void)</span><br><span> {</span><br><span>@@ -517,11 +494,34 @@</span><br><span> /*! Provides the moh channel with a thread so it can actually play its music */</span><br><span> static void *moh_channel_thread(void *data)</span><br><span> {</span><br><span style="color: hsl(0, 100%, 40%);">- struct ast_channel *moh_channel = data;</span><br><span style="color: hsl(120, 100%, 40%);">+ struct stasis_app_bridge_channel_wrapper *moh_wrapper = data;</span><br><span style="color: hsl(120, 100%, 40%);">+ struct ast_channel *moh_channel = ast_channel_get_by_name(moh_wrapper->channel_id);</span><br><span style="color: hsl(120, 100%, 40%);">+ struct ast_frame *f;</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- while (!ast_safe_sleep(moh_channel, 1000)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!moh_channel) {</span><br><span style="color: hsl(120, 100%, 40%);">+ ao2_unlink(app_bridges_moh, moh_wrapper);</span><br><span style="color: hsl(120, 100%, 40%);">+ ao2_ref(moh_wrapper, -1);</span><br><span style="color: hsl(120, 100%, 40%);">+ return NULL;</span><br><span> }</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+ /* Read and discard any frame coming from the stasis bridge. */</span><br><span style="color: hsl(120, 100%, 40%);">+ for (;;) {</span><br><span style="color: hsl(120, 100%, 40%);">+ if (ast_waitfor(moh_channel, -1) < 0) {</span><br><span style="color: hsl(120, 100%, 40%);">+ /* Error or hungup */</span><br><span style="color: hsl(120, 100%, 40%);">+ break;</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ f = ast_read(moh_channel);</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!f) {</span><br><span style="color: hsl(120, 100%, 40%);">+ /* Hungup */</span><br><span style="color: hsl(120, 100%, 40%);">+ break;</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_frfree(f);</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ ao2_unlink(app_bridges_moh, moh_wrapper);</span><br><span style="color: hsl(120, 100%, 40%);">+ ao2_ref(moh_wrapper, -1);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> ast_moh_stop(moh_channel);</span><br><span> ast_hangup(moh_channel);</span><br><span> </span><br><span>@@ -539,15 +539,10 @@</span><br><span> */</span><br><span> static struct ast_channel *bridge_moh_create(struct ast_bridge *bridge)</span><br><span> {</span><br><span style="color: hsl(0, 100%, 40%);">- RAII_VAR(struct stasis_app_bridge_channel_wrapper *, new_wrapper, NULL, ao2_cleanup);</span><br><span style="color: hsl(0, 100%, 40%);">- RAII_VAR(char *, bridge_id, ast_strdup(bridge->uniqueid), ast_free);</span><br><span style="color: hsl(120, 100%, 40%);">+ struct stasis_app_bridge_channel_wrapper *new_wrapper;</span><br><span> struct ast_channel *chan;</span><br><span> pthread_t threadid;</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- if (!bridge_id) {</span><br><span style="color: hsl(0, 100%, 40%);">- return NULL;</span><br><span style="color: hsl(0, 100%, 40%);">- }</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span> chan = prepare_bridge_moh_channel();</span><br><span> if (!chan) {</span><br><span> return NULL;</span><br><span>@@ -558,14 +553,6 @@</span><br><span> return NULL;</span><br><span> }</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- /* The after bridge callback assumes responsibility of the bridge_id. */</span><br><span style="color: hsl(0, 100%, 40%);">- if (ast_bridge_set_after_callback(chan,</span><br><span style="color: hsl(0, 100%, 40%);">- moh_after_bridge_cb, moh_after_bridge_cb_failed, bridge_id)) {</span><br><span style="color: hsl(0, 100%, 40%);">- ast_hangup(chan);</span><br><span style="color: hsl(0, 100%, 40%);">- return NULL;</span><br><span style="color: hsl(0, 100%, 40%);">- }</span><br><span style="color: hsl(0, 100%, 40%);">- bridge_id = NULL;</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span> if (ast_unreal_channel_push_to_bridge(chan, bridge,</span><br><span> AST_BRIDGE_CHANNEL_FLAG_IMMOVABLE | AST_BRIDGE_CHANNEL_FLAG_LONELY)) {</span><br><span> ast_hangup(chan);</span><br><span>@@ -579,21 +566,25 @@</span><br><span> return NULL;</span><br><span> }</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- if (ast_string_field_init(new_wrapper, 32)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ if (ast_string_field_init(new_wrapper, AST_UUID_STR_LEN + AST_CHANNEL_NAME)</span><br><span style="color: hsl(120, 100%, 40%);">+ || ast_string_field_set(new_wrapper, bridge_id, bridge->uniqueid)</span><br><span style="color: hsl(120, 100%, 40%);">+ || ast_string_field_set(new_wrapper, channel_id, ast_channel_uniqueid(chan))) {</span><br><span style="color: hsl(120, 100%, 40%);">+ ao2_ref(new_wrapper, -1);</span><br><span> ast_hangup(chan);</span><br><span> return NULL;</span><br><span> }</span><br><span style="color: hsl(0, 100%, 40%);">- ast_string_field_set(new_wrapper, bridge_id, bridge->uniqueid);</span><br><span style="color: hsl(0, 100%, 40%);">- ast_string_field_set(new_wrapper, channel_id, ast_channel_uniqueid(chan));</span><br><span> </span><br><span> if (!ao2_link_flags(app_bridges_moh, new_wrapper, OBJ_NOLOCK)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ ao2_ref(new_wrapper, -1);</span><br><span> ast_hangup(chan);</span><br><span> return NULL;</span><br><span> }</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- if (ast_pthread_create_detached(&threadid, NULL, moh_channel_thread, chan)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ /* Pass the new_wrapper ref to moh_channel_thread() */</span><br><span style="color: hsl(120, 100%, 40%);">+ if (ast_pthread_create_detached(&threadid, NULL, moh_channel_thread, new_wrapper)) {</span><br><span> ast_log(LOG_ERROR, "Failed to create channel thread. Abandoning MOH channel creation.\n");</span><br><span> ao2_unlink_flags(app_bridges_moh, new_wrapper, OBJ_NOLOCK);</span><br><span style="color: hsl(120, 100%, 40%);">+ ao2_ref(new_wrapper, -1);</span><br><span> ast_hangup(chan);</span><br><span> return NULL;</span><br><span> }</span><br><span>@@ -2200,8 +2191,8 @@</span><br><span> app_controls = ao2_container_alloc(CONTROLS_NUM_BUCKETS, control_hash, control_compare);</span><br><span> app_bridges = ao2_container_alloc(BRIDGES_NUM_BUCKETS, bridges_hash, bridges_compare);</span><br><span> app_bridges_moh = ao2_container_alloc_hash(</span><br><span style="color: hsl(0, 100%, 40%);">- AO2_ALLOC_OPT_LOCK_MUTEX, AO2_CONTAINER_ALLOC_OPT_DUPS_REJECT,</span><br><span style="color: hsl(0, 100%, 40%);">- 37, bridges_channel_hash_fn, bridges_channel_sort_fn, NULL);</span><br><span style="color: hsl(120, 100%, 40%);">+ AO2_ALLOC_OPT_LOCK_MUTEX, 0,</span><br><span style="color: hsl(120, 100%, 40%);">+ 37, bridges_channel_hash_fn, NULL, NULL);</span><br><span> app_bridges_playback = ao2_container_alloc_hash(</span><br><span> AO2_ALLOC_OPT_LOCK_MUTEX, AO2_CONTAINER_ALLOC_OPT_DUPS_REJECT,</span><br><span> 37, bridges_channel_hash_fn, bridges_channel_sort_fn, NULL);</span><br><span></span><br></pre><p>To view, visit <a href="https://gerrit.asterisk.org/9297">change 9297</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/9297"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 15 </div>
<div style="display:none"> Gerrit-MessageType: merged </div>
<div style="display:none"> Gerrit-Change-Id: Ibff479e167b3320c68aaabfada7e1d0ef7bd548c </div>
<div style="display:none"> Gerrit-Change-Number: 9297 </div>
<div style="display:none"> Gerrit-PatchSet: 7 </div>
<div style="display:none"> Gerrit-Owner: Moritz Fain <moritz@fain.io> </div>
<div style="display:none"> Gerrit-Reviewer: Benjamin Keith Ford <bford@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 (1000185) </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Matthew Fredrickson <creslin@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Moritz Fain <moritz@fain.io> </div>
<div style="display:none"> Gerrit-Reviewer: Pascal Cadotte Michaud <pcm@wazo.io> </div>
<div style="display:none"> Gerrit-Reviewer: Richard Mudgett <rmudgett@digium.com> </div>