channel.c: Move setting RTP stats from ast_softhangup to ast_ari_channels_hangup.

The original trigger for setting the RTP stats in ast_softhangup() came from
an ARI issue where stats weren't being set in time to be reported on STASIS_END
events. The thought was that setting them in a common place like ast_softhangup()
would ensure the stats were set in possibly other scenarios.  Unfortunately,
setting the RTP stats variables in ast_softhangup() broke ABI as it required
that no channel locks be held which was not the case earlier.

Given that the original issue was ARI, we can move setting the stats to
ast_ari_channels_hangup() in resource_channels just before it calls
ast_softhangup().  This might not catch all cases of the stats not being set,
but it won't break ABI or deadlock either.

Resolves: #1928

diff --git a/include/asterisk/channel.h b/include/asterisk/channel.h
index c24f8ed6e41980746727b433a9e3eb8f698b3812..90a7dc293d64036d820fa0bcd5f089c643f9e792 100644 (file)
--- a/include/asterisk/channel.h
+++ b/include/asterisk/channel.h
@@ -1654,14 +1654,7 @@ void ast_softhangup_all(void);
  * (use this if you are trying to
  * safely hangup a channel managed by another thread.
  *
- * \warning The channel passed to this function must NOT be locked.
- * ast_softhangup() calls ast_rtp_instance_set_stats_vars() to set RTP QOS variables.
- * If this channel is in a bridge, ast_rtp_instance_set_stats_vars() will
- * attempt to lock the bridge peer as well as this channel.  This can cause
- * a lock inversion if we already have this channel locked and another
- * thread tries to set bridge variables on the peer because it will have
- * locked the peer first, then this channel.  For this reason, we must
- * NOT have the channel locked when we call ast_softhangup().
+ * \note The channel passed to this function does not need to be locked.
  *
  * \return Returns 0 regardless
  */

diff --git a/main/channel.c b/main/channel.c
index 146e2115ee2268e69e2053f8f82aaab77a2c7aef..a305978e014299739a3c75483e1203b68e9129c7 100644 (file)
--- a/main/channel.c
+++ b/main/channel.c
@@ -2464,41 +2464,7 @@ int ast_softhangup(struct ast_channel *chan, int cause)
        RAII_VAR(struct ast_json *, blob, NULL, ast_json_unref);
        int res;
        int tech_cause = 0;
-       struct ast_rtp_glue *glue;
-       struct ast_rtp_instance *rtp = NULL;
-       const struct ast_channel_tech *tech;
-
-       /*
-        * Only hold the channel lock long enough to get the rtp instance.
-        * glue->get_rtp_info() will bump the refcount on it.
-        */
-       ast_channel_lock(chan);
-       tech = ast_channel_tech(chan);
-       glue = ast_rtp_instance_get_glue(tech->type);
-       if (glue) {
-               glue->get_rtp_info(chan, &rtp);
-       }
-       ast_channel_unlock(chan);
 
-       /*
-        * If this channel is in a bridge, ast_rtp_instance_set_stats_vars() will
-        * attempt to lock the bridge peer as well as this channel.  This can cause
-        * a lock inversion if we already have this channel locked and another
-        * thread tries to set bridge variables on the peer because it will have
-        * locked the peer first, then this channel.  For this reason, we must
-        * NOT have the channel locked when we call ast_rtp_instance_set_stats_vars().
-        * This should be safe since glue->get_rtp_info() will have bumped the
-        * refcount on the rtp instance so it can't go away while the channel
-        * is unlocked.
-        */
-       if (rtp) {
-               ast_rtp_instance_set_stats_vars(chan, rtp);
-               ao2_ref(rtp, -1);
-       }
-
-       /*
-        * Now it's safe to lock the channel again.
-        */
        ast_channel_lock(chan);
 
        res = ast_softhangup_nolock(chan, cause);

diff --git a/res/ari/resource_channels.c b/res/ari/resource_channels.c
index 1077eebd493f9c6de85b1bc99700e2d460ff346a..527803e952db4d7f6c15ab98ab00ce5888e04f87 100644 (file)
--- a/res/ari/resource_channels.c
+++ b/res/ari/resource_channels.c
@@ -931,6 +931,9 @@ void ast_ari_channels_hangup(struct ast_variable *headers,
 {
        RAII_VAR(struct ast_channel *, chan, NULL, ao2_cleanup);
        int cause;
+       struct ast_rtp_glue *glue;
+       struct ast_rtp_instance *rtp = NULL;
+       const struct ast_channel_tech *tech;
 
        chan = ast_channel_get_by_name(args->channel_id);
        if (chan == NULL) {
@@ -969,6 +972,34 @@ void ast_ari_channels_hangup(struct ast_variable *headers,
        }
 
        ast_channel_hangupcause_set(chan, cause);
+
+       /*
+        * Only hold the channel lock long enough to get the rtp instance.
+        * glue->get_rtp_info() will bump the refcount on it.
+        */
+       ast_channel_lock(chan);
+       tech = ast_channel_tech(chan);
+       glue = ast_rtp_instance_get_glue(tech->type);
+       if (glue) {
+               glue->get_rtp_info(chan, &rtp);
+       }
+       ast_channel_unlock(chan);
+       /*
+        * If this channel is in a bridge, ast_rtp_instance_set_stats_vars() will
+        * attempt to lock the bridge peer as well as this channel.  This can cause
+        * a lock inversion if we already have this channel locked and another
+        * thread tries to set bridge variables on the peer because it will have
+        * locked the peer first, then this channel.  For this reason, we must
+        * NOT have the channel locked when we call ast_rtp_instance_set_stats_vars().
+        * This should be safe since glue->get_rtp_info() will have bumped the
+        * refcount on the rtp instance so it can't go away while the channel
+        * is unlocked.
+        */
+       if (rtp) {
+               ast_rtp_instance_set_stats_vars(chan, rtp);
+               ao2_ref(rtp, -1);
+       }
+
        ast_softhangup(chan, AST_SOFTHANGUP_EXPLICIT);
 
        ast_ari_response_no_content(response);