[Asterisk-code-review] Fixed locking issue in stasis app bridge moh channel (asterisk[15])

[Moritz Fain]

Moritz Fain has posted comments on this change. ( https://gerrit.asterisk.org/9297 )

Change subject: Fixed locking issue in stasis_app_bridge_moh_channel
......................................................................


Patch Set 1:

(7 comments)

https://gerrit.asterisk.org/#/c/9297/1/res/res_stasis.c
File res/res_stasis.c:

https://gerrit.asterisk.org/#/c/9297/1/res/res_stasis.c@521
PS1, Line 521: 	struct ast_channel *moh_channel = ast_channel_get_by_name(moh_wrapper->channel_id);
> For safety you need to check if moh_channel is NULL and unlink/unref moh_wr
Correct. I will just return in case this is NULL. Possible but should not happen to often if the thread starts right away. Still possible :)


https://gerrit.asterisk.org/#/c/9297/1/res/res_stasis.c@526
PS1, Line 526: 	ao2_lock(app_bridges_moh);
             : 	ao2_unlink_flags(app_bridges_moh, moh_wrapper, OBJ_NOLOCK);
             : 
             : 	ast_moh_stop(moh_channel);
             : 	ast_hangup(moh_channel);
             : 
             : 	ao2_unlock(app_bridges_moh);
> You just need to unlink moh_wrapper from the container and unref moh_wrappe
Yes!


https://gerrit.asterisk.org/#/c/9297/1/res/res_stasis.c@567
PS1, Line 567: 	/* 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;
> This is dead code and needs to be removed along with the passed in callback
When are these callbacks going to be called? Sounds like when the bridge is destroyed. And the callback removes the MOH announcer channel. Not to bad?

Is this called AFTER the threads exits? Then it might be better to do the cleanup here rather than in the thread?


https://gerrit.asterisk.org/#/c/9297/1/res/res_stasis.c@588
PS1, Line 588: 	if (ast_string_field_init(new_wrapper, 32)) {
> The initial size should be increased as we are likely to always need more s
Yes


https://gerrit.asterisk.org/#/c/9297/1/res/res_stasis.c@588
PS1, Line 588: 	if (ast_string_field_init(new_wrapper, 32)) {
             : 		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));
> Since we are passing new_wrapper to moh_channel_thread() these string field
Yes


https://gerrit.asterisk.org/#/c/9297/1/res/res_stasis.c@600
PS1, Line 600: 	if (ast_pthread_create_detached(&threadid, NULL, moh_channel_thread, new_wrapper)) {
> We need to pass a new_wrapper ref to moh_channel_thread().  We cannot rely 
Ok, will fix that. I'm not deep enough into ao2 stuff.
Basically it's just updating the ref counter so that the wrapper will not get de-allocated?


https://gerrit.asterisk.org/#/c/9297/1/res/res_stasis.c@625
PS1, Line 625: 	ao2_unlock(app_bridges_moh);
> Moving this lock doesn't help.  As soon as you release the lock someone cou
It's not about locking the channel. It's about locking the wrapper.

If I move the unlock where it was I end up having a moh_wrapper structure (in line 621) whose channel might not be there any more.

That's why I moved the unlock at the end.



-- 
To view, visit https://gerrit.asterisk.org/9297
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 15
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibff479e167b3320c68aaabfada7e1d0ef7bd548c
Gerrit-Change-Number: 9297
Gerrit-PatchSet: 1
Gerrit-Owner: Moritz Fain <moritz at fain.io>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Moritz Fain <moritz at fain.io>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Wed, 27 Jun 2018 10:38:38 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180627/e639e834/attachment-0001.html>