[Asterisk-code-review] app_confbridge: Fix bridge shutdown race condition. (asterisk[master])

[N A]

Attention is currently required from: Joshua Colp.
N A has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/18577 )

Change subject: app_confbridge: Fix bridge shutdown race condition.
......................................................................


Patch Set 1:

(3 comments)

File apps/app_confbridge.c:

https://gerrit.asterisk.org/c/asterisk/+/18577/comment/e127970e_4dd8bca6 
PS1, Line 1742: 	conference = ao2_find(conference_bridges, conference_name, OBJ_KEY);
> The underlying issue here is that the conference bridge has transitioned to empty and another thread […]
I think I follow this logic except for one caveat: what about the case where the conference is *not* empty when we find it here, but becomes empty in the middle here before we actually join it (that is, by the time we execute that other code you commented on, it becomes empty and is then cleaned up).
This seems like another potential race condition to me, just in a different way/place - or did I miss something else?


https://gerrit.asterisk.org/c/asterisk/+/18577/comment/61c11870_ad1abf60 
PS1, Line 1906: 	/* Rare, but if threads interleave exactly right, the bridge could disappear
> I don't think this is a complete fix. […]
I think the code at this point is foolproof, because this is the point where we try to enter the bridge, with the entire lock held, so if we detect that it disappeared on us, we can remediate it at this point.

I think the larger issue here is not that the code might not work, but the fact that the code is even needed in the first place - it does feel somewhat hacky to have code effectively like "do A" (which should always succeeded, in theory), and then "if A failed, let's make sure A succeeded by doing B".


https://gerrit.asterisk.org/c/asterisk/+/18577/comment/19c5fa55_9ffe69f8 
PS1, Line 1915: 			conference->bridge, conference2 ? "changed" : "disappeared");
> It's not possible for conference->bridge to be NULL here.
The ternary is on conference2 which could be NULL.



-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/18577
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: I08a440eafbf83ec4b502d1e44c3f4d44c4a522f9
Gerrit-Change-Number: 18577
Gerrit-PatchSet: 1
Gerrit-Owner: N A <mail at interlinked.x10host.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Attention: Joshua Colp <jcolp at sangoma.com>
Gerrit-Comment-Date: Sat, 06 Aug 2022 20:24:41 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Joshua Colp <jcolp at sangoma.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20220806/9474278b/attachment.html>