kenmirrors/asterisk
1
0
Fork 0
mirror of https://github.com/asterisk/asterisk.git synced 2026-06-30 20:19:42 -07:00
Code Issues Packages Projects Releases Wiki Activity

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

Browse Source 
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
This commit is contained in:
George Joseph
2026-05-12 18:30:04 -06:00
parent deb8a51a26
commit 2fedb5d195
3 changed files with 32 additions and 42 deletions
Download Patch File Download Diff File Expand all files Collapse all files
include/asterisk/channel.h
+1 -8
View File
@@ -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
*/
main/channel.c
-34
View File
@@ -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);
res/ari/resource_channels.c
+31
View File
@@ -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);