channels: Avoid deadlocks when locking ao2_container *channels #311

intellasoft · 2023-09-09T19:10:38Z

One situation where deadlocks occur is when:
-> res_pjsip_sdp_rtp is checking for rtp timeouts -> Which locks
ao2_container channels, and then waits for a lock on a channel
that's currently running IMPORT
At the same time
-> Dialplan asks for IMPORT(${some_channel},...) -> Which grabs a
channel lock and then waits on ao2_container channels

So in this situation, res_pjsip_sdp_rtp is waiting on a channel lock
that will never release, because res_pjsip_sdp_rtp is currently
holding ao2_container channels, that the IMPORT needs after having
already locked the channel that res_pjsip_sdp_rtp is processing next.

Resolves: #307

cherry-pick-to: 19
cherry-pick-to: 20
cherry-pick-to: master

asterisk-org-access-app · 2023-09-09T19:12:56Z

REMINDER: If this PR applies to other branches, please add a comment with the appropriate "cherry-pick-to" headers as per the Create a Pull Request process.

If you don't want it cherry-picked, please add a comment with cherry-pick-to: none so we don't keep asking.

If, after adding "cherry-pick-to" comments, you change your mind, please edit the comment to DELETE the header lines and add cherry-pick-to: none.

The currently active branches are now 18, 20, 21 and master.

InterLinked1

Remove the cherry pick lines from the commit message and make a comment with those instead.

InterLinked1 · 2023-09-09T19:12:39Z

main/channel.c

@@ -712,12 +718,23 @@ static int does_id_conflict(const char *uniqueid)
 {
 	struct ast_channel *conflict;
 	size_t length = 0;
+	struct ast_channel_callback_data callback_data = {.search = uniqueid, .final_match_result = 0};


Can we put each element on its own line? Not sure if that's explicitly in the coding guidelines but that's easier to read.

InterLinked1 · 2023-09-09T19:13:53Z

main/channel.c


 	if (ast_strlen_zero(uniqueid)) {
 		return 0;
 	}

-	conflict = ast_channel_callback(ast_channel_by_uniqueid_cb, (char *) uniqueid, &length, OBJ_NOLOCK);
+  try_again:


I don't think goto is necessary here, this could be a do while or for (;;) with break

InterLinked1 · 2023-09-09T19:15:45Z

main/channel.c

+
+	if (callback_data.final_match_result == CMP_RETRY_NEEDED) {
+		/* We ran into a possible deadlock, try again */
+		ast_debug(3, "ast_channel_iterator_by_exten_new() Avoid deadlock trying to find channel by extension@context: %s@%s\n", exten, context);


ast_debug includes the function name, no need to repeat it.

This could all be simplified to:

`ast_debug(3, "Avoided deadlock trying to find channel by %@%s\n", exten, context);

InterLinked1 · 2023-09-09T19:16:11Z

main/channel.c


 	if (ast_strlen_zero(l_name)) {
 		/* We didn't have a name to search for so quit. */
 		return NULL;
 	}

-	chan = ast_channel_callback(ast_channel_by_name_cb, l_name, &name_len,
+  try_name_again:


Same thing here, you can use 2 loops here and goto isn't needed.

Are goto's that bad? It reduces nesting. The only thing that adding a loop does is introduce an extra level of nesting.

They're not inherently bad, this just doesn't seem like a necessary or appropriate use for them, since a loop works perfectly fine. I don't think there's a problem with adding a level of nesting, it contains the logic in its own scope.

InterLinked1 · 2023-09-09T19:16:19Z

main/channel.c

+
+	if (callback_data.final_match_result == CMP_RETRY_NEEDED) {
+		/* We ran into a possible deadlock, try again */
+		ast_debug(3, "ast_channel_get_by_name_prefix() Avoid deadlock trying to find channel by uniqueid: %s\n", name);


Same comment about log format

kobaz · 2023-09-09T19:21:31Z

cherry-pick-to: 19
cherry-pick-to: 20
cherry-pick-to: master

InterLinked1 · 2023-09-09T19:25:41Z

cherry-pick-to: 19
cherry-pick-to: 20
cherry-pick-to: master

PRs should be raised against master, not 18 (unlike Gerrit, it makes a difference now).
And it should be cherry picked to 18, 20, and 21, not 19.

kobaz · 2023-09-09T19:27:27Z

master is slightly different. It will not cleanly cherry-pick to branches from master. Is the process still go to to master first? Make separate PRs for each branch?

InterLinked1 · 2023-09-09T19:30:04Z

master is slightly different. It will not cleanly cherry-pick to branches from master. Is the process still go to to master first? Make separate PRs for each branch?

Ah, okay, then it's more complicated, then you might need to do multiple sets since GitHub can only cherry pick verbatim, might want to double check with George.

kobaz · 2023-09-10T17:03:24Z

master is slightly different. It will not cleanly cherry-pick to branches from master. Is the process still go to to master first? Make separate PRs for each branch?

Ah, okay, then it's more complicated, then you might need to do multiple sets since GitHub can only cherry pick verbatim, might want to double check with George.

master no longer has support for macros, so the checks in ast_channel_by_exten_cb() will not cherry-pick clean with the change as-is.

InterLinked1 · 2023-09-10T16:49:16Z

main/channel.c

+		/* We found a match or the search fully completed through all channels */
+		break;
+	}
+	while (1);


while should go on previous line
Though really I think for(;;) is preferred in Asterisk for infinite loops.

InterLinked1 · 2023-09-10T17:16:14Z

main/channel.c

+		/* We found a match or the search fully completed through all channels */
+		break;
+	}
+	while (1);


InterLinked1 · 2023-09-10T17:17:00Z

main/channel.c

+		/* We found a match or the search fully completed through all channels */
+		break;
+	}
+	while (1);


kobaz · 2023-09-21T14:49:24Z

Update: Been running this in the wild with mixed results.

Some days it's been completely solid with no deadlocks (better than before with 3-5 deadlocks per day or more).

Some days it actually almost immediately deadlocked after handling just a few calls.

jcolp · 2023-09-29T13:43:42Z

Due to the last comment I am transitioning this to a draft as it does not appear to be in a state for review as it does not resolve the underlying issue. If this is untrue and it should actually be reviewed, then it can be published.

kobaz · 2023-09-30T03:06:57Z

Fixed previous issue where it did not fully fix the problem. Small refactor to avoid breaking assumptions about value of 'arg' with respect to find channel callbacks.

It's now passing my tests regarding the original deadlock situation.

I would nominate this ready for review now.

One situation where deadlocks occur is when: -> res_pjsip_sdp_rtp is checking for rtp timeouts -> Which locks ao2_container channels, and then waits for a lock on a channel that's currently running IMPORT At the same time -> Dialplan asks for IMPORT(${some_channel},...) -> Which grabs a channel lock and then waits on ao2_container channels So in this situation, res_pjsip_sdp_rtp is waiting on a channel lock that will never release, because res_pjsip_sdp_rtp is currently holding ao2_container channels, that the IMPORT needs after having already locked the channel that res_pjsip_sdp_rtp is processing next. UserNote: Module developers should discontinue use of ast_channel_callback() and use ast_channel_callback_safe() instead. (This will avoid causing deadlocks) Resolves: asterisk#307

kobaz · 2023-10-08T15:27:44Z

Pushed a new one.. fixes channel reference leaks (introduced in the previous patch) when OBJ_MULTIPLE is used to return an iterator

kobaz · 2023-10-08T16:47:59Z

While trying to extract out components from our full platform into a standalone deadlock reproduction test, I've uncovered an additional deadlock related to ast_sip_push_task_wait() also related to IMPORT import_helper() (posted to #307). I realized this is not the same deadlock that this #311 pull is addressing. So I'll work on another test to trigger this specific one as well.

This is related, but probably should be split into a new issue since this other newly uncovered deadlock is not related to ao2_container *channels.

gtjoseph · 2024-01-23T16:42:08Z

I'm converting this back to a draft again since there are still outstanding changes requested and the cherry-pick lines still aren't correct.

Speaking of which, are you saying this PR will cherry-pick cleanly from 18 to master but not the other way around?

kobaz · 2024-01-23T16:43:35Z

Correct, I don't think it goes the other way, but I can do some tests.

gtjoseph · 2025-06-11T12:45:31Z

@intellasoft Is this PR still relevant since the channel storage backends work was merged?

intellasoft · 2025-06-11T12:47:46Z

That's a really good question. This can probably be put on hold for now... I'll need to validate if this still happens with the new backend.

asterisk-org-access-app bot requested a review from a team September 9, 2023 19:12

github-actions bot added testing-in-progress test-checks-failed and removed testing-in-progress labels Sep 9, 2023

InterLinked1 suggested changes Sep 9, 2023

View reviewed changes

intellasoft force-pushed the 18-issue-307 branch from 8fa1b68 to e200e31 Compare September 9, 2023 19:19

github-actions bot added testing-in-progress test-checks-failed and removed test-checks-failed testing-in-progress labels Sep 9, 2023

intellasoft force-pushed the 18-issue-307 branch from e200e31 to 52184d3 Compare September 10, 2023 16:47

github-actions bot added testing-in-progress test-checks-failed and removed test-checks-failed testing-in-progress labels Sep 10, 2023

InterLinked1 suggested changes Sep 10, 2023

View reviewed changes

jcolp marked this pull request as draft September 29, 2023 13:43

intellasoft force-pushed the 18-issue-307 branch from 52184d3 to 520b377 Compare September 30, 2023 03:04

github-actions bot added testing-in-progress and removed test-checks-failed labels Sep 30, 2023

github-actions bot added the test-checks-passed label Sep 30, 2023

intellasoft force-pushed the 18-issue-307 branch from 274087e to 59b0d33 Compare October 3, 2023 18:44

github-actions bot added testing-in-progress test-checks-passed test-gates-failed and removed test-checks-passed test-gates-failed testing-in-progress labels Oct 3, 2023

intellasoft force-pushed the 18-issue-307 branch from 59b0d33 to a000ddc Compare October 4, 2023 13:10

github-actions bot added testing-in-progress test-checks-passed test-gates-failed and removed test-checks-passed test-gates-failed testing-in-progress labels Oct 4, 2023

intellasoft force-pushed the 18-issue-307 branch from a000ddc to 3579cc5 Compare October 8, 2023 15:26

github-actions bot added testing-in-progress test-checks-passed test-gates-failed and removed test-checks-passed test-gates-failed testing-in-progress labels Oct 8, 2023

gtjoseph marked this pull request as draft January 23, 2024 16:42

gtjoseph assigned intellasoft Jan 23, 2024

channels: Avoid deadlocks when locking ao2_container *channels #311

Are you sure you want to change the base?

channels: Avoid deadlocks when locking ao2_container *channels #311

Conversation

intellasoft commented Sep 9, 2023

Uh oh!

asterisk-org-access-app bot commented Sep 9, 2023

Uh oh!

InterLinked1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kobaz commented Sep 9, 2023

Uh oh!

InterLinked1 commented Sep 9, 2023

Uh oh!

kobaz commented Sep 9, 2023

Uh oh!

InterLinked1 commented Sep 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kobaz commented Sep 10, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kobaz commented Sep 21, 2023

Uh oh!

jcolp commented Sep 29, 2023

Uh oh!

kobaz commented Sep 30, 2023

Uh oh!

kobaz commented Oct 8, 2023

Uh oh!

kobaz commented Oct 8, 2023

Uh oh!

gtjoseph commented Jan 23, 2024

Uh oh!

kobaz commented Jan 23, 2024

Uh oh!

gtjoseph commented Jun 11, 2025

Uh oh!

intellasoft commented Jun 11, 2025

Uh oh!

Uh oh!

InterLinked1 commented Sep 9, 2023 •

edited

Loading