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