<p>Richard Mudgett <strong>posted comments</strong> on this change.</p><p><a href="https://gerrit.asterisk.org/9297">View Change</a></p><p>Patch set 1:</p><p>(4 comments)</p><ul style="list-style: none; padding-left: 20px;"><li><p><a href="https://gerrit.asterisk.org/#/c/9297/1/res/res_stasis.c">File res/res_stasis.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9297/1/res/res_stasis.c@521">Patch Set #1, Line 521:</a> <code style="font-family:monospace,monospace"> struct ast_channel *moh_channel = ast_channel_get_by_name(moh_wrapper->channel_id);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Correct. I will just return in case this is NULL. Possible but should not h</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">For safety you need to do the below if you don't get the channel:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">if (!moh_channel) {<br> ao2_unlink(app_bridges_moh, moh_wrapper);<br> ao2_ref(moh_wrapper, -1);<br> return NULL;<br>}</pre></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9297/1/res/res_stasis.c@567">Patch Set #1, Line 567:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> /* The after bridge callback assumes responsibility of the bridge_id. */<br> if (ast_bridge_set_after_callback(chan,<br> moh_after_bridge_cb, moh_after_bridge_cb_failed, bridge_id)) {<br> ast_hangup(chan);<br> return NULL;<br> }<br> bridge_id = NULL;<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">When are these callbacks going to be called? Sounds like when the bridge is</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">We are attaching the callbacks to chan which is the ;1 channel of the unreal channel pair. We should have attached them to the ;2 channel which is pushed into the bridge by ast_unreal_channel_push_to_bridge(). The callbacks are called when the specific channel leaves the bridging system not when the bridge is destroyed. Since the ;1 channel is never put into a bridge the callbacks never get called. This is the main reason causing the issue because the wrapper is not removed from the app_bridges_moh container.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Your patch moves things around so we do not need to use the after bridge callbacks. This is why the code is now dead code.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9297/1/res/res_stasis.c@600">Patch Set #1, Line 600:</a> <code style="font-family:monospace,monospace"> if (ast_pthread_create_detached(&threadid, NULL, moh_channel_thread, new_wrapper)) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Ok, will fix that. I'm not deep enough into ao2 stuff.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">correct</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9297/1/res/res_stasis.c@625">Patch Set #1, Line 625:</a> <code style="font-family:monospace,monospace"> ao2_unlock(app_bridges_moh);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">It's not about locking the channel. It's about locking the wrapper.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I said nothing about locking the channel. Calling ast_channel_get_by_name() could return a dead channel if you hold the container lock. Even if the channel is live when you get it that channel could die at any time. Holding the wrapper container lock does not prevent that.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Are you thinking that it is better to get a dead channel to attempt starting MOH on? Or maybe if we cannot find the channel we should loop around and try creating the channel again?</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/9297">change 9297</a>. To unsubscribe, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/9297"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 15 </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: Ibff479e167b3320c68aaabfada7e1d0ef7bd548c </div>
<div style="display:none"> Gerrit-Change-Number: 9297 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Moritz Fain <moritz@fain.io> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 </div>
<div style="display:none"> Gerrit-Reviewer: Moritz Fain <moritz@fain.io> </div>
<div style="display:none"> Gerrit-Reviewer: Richard Mudgett <rmudgett@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 27 Jun 2018 16:07:47 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>