<p> Attention is currently required from: Joshua Colp. </p>
<p><a href="https://gerrit.asterisk.org/c/asterisk/+/15985">View Change</a></p><p>1 comment:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="null">File apps/app_dial.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/15985/comment/ed92d4ae_f459f720">Patch Set #2, Line 1802:</a> <code style="font-family:monospace,monospace">                                           if (ast_check_hangup_locked(o->chan)) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">The issue is that it hasn't been explained precisely why it's the correct fix. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">The existing logic fails because it's possible for a channel (that exists) to have a soft hangup queued for it. We have an indication that we should give up, but it's just not one of the checks being done currently.</p><p style="white-space: pre-wrap; word-wrap: break-word;">It seems to be an issue only when the channel hasn't yet been answered (e.g. progress DTMF or progress MF) - and furthermore, if you wait at least 80ms, after digits have been sent, then we're still good, even then. I don't think it's specifically tone related, just perhaps that app_dial is busy sending digits and perhaps missing something that arrives within that 80ms period, or perhaps it's just too slow at something.<br>Outside of these constraints, I haven't noticed any improper handling. There may just be other cases that haven't presented themselves yet. I don't know if the tone on progress is the actual cause or just a symptom of the actual issue.</p><p style="white-space: pre-wrap; word-wrap: break-word;">We know the issue is that a soft hangup is queued, but the channel still exists and frames are still being processed from/for the dead channel.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I don't think it's a perfect solution, still, but given that we know that it works, what would be the harm in including something like:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">if (ast_check_hangup_locked(o->chan)) {<br>    ast_log(LOG_WARNING, "Something went wrong, channel failed to be killed properly, bailing out now\n");<br>      continue;<br>}</pre><p style="white-space: pre-wrap; word-wrap: break-word;">or</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">if (ast_channel_softhangup_internal_flag(o->chan)) {<br>    ast_log(LOG_WARNING, "Something went wrong, channel failed to be killed properly, bailing out now\n");<br>      continue;<br>}</pre><p style="white-space: pre-wrap; word-wrap: break-word;">It doesn't seem like it would be a performance issue, since all that it's doing is accessing o->chan->softhangup.</p><p style="white-space: pre-wrap; word-wrap: break-word;">The reason this fix works is that it catches hangups that don't set the channel to null, and where ast_read doesn't return null. What I don't know is if this is something that should ever happen - is this a valid state for a channel to be in? But presumably it is, right? Otherwise, why would this function exist in the first place? Or is it just not supposed to be necessary for app_dial, and since we're not currently checking it, this is happening?</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/15985">change 15985</a>. To unsubscribe, or for help writing mail filters, 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/c/asterisk/+/15985"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I6f2ee3f77b892015bc05513d868f071f279a3c80 </div>
<div style="display:none"> Gerrit-Change-Number: 15985 </div>
<div style="display:none"> Gerrit-PatchSet: 2 </div>
<div style="display:none"> Gerrit-Owner: N A <mail@interlinked.x10host.com> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@sangoma.com> </div>
<div style="display:none"> Gerrit-Attention: Joshua Colp <jcolp@sangoma.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Fri, 06 Aug 2021 23:09:19 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Comment-In-Reply-To: Joshua Colp <jcolp@sangoma.com> </div>
<div style="display:none"> Comment-In-Reply-To: N A <mail@interlinked.x10host.com> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>