[Asterisk-code-review] app_dial: Prevent call from hanging (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/+/15985 )

Change subject: app_dial: Prevent call from hanging
......................................................................


Patch Set 2:

(1 comment)

File apps/app_dial.c:

https://gerrit.asterisk.org/c/asterisk/+/15985/comment/ed92d4ae_f459f720 
PS2, Line 1802: 						if (ast_check_hangup_locked(o->chan)) {
> The issue is that it hasn't been explained precisely why it's the correct fix. […]
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.

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.
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.

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.

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:

if (ast_check_hangup_locked(o->chan)) {
	ast_log(LOG_WARNING, "Something went wrong, channel failed to be killed properly, bailing out now\n");
	continue;
}

or

if (ast_channel_softhangup_internal_flag(o->chan)) {
	ast_log(LOG_WARNING, "Something went wrong, channel failed to be killed properly, bailing out now\n");
	continue;
}

It doesn't seem like it would be a performance issue, since all that it's doing is accessing o->chan->softhangup.

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?



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

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: I6f2ee3f77b892015bc05513d868f071f279a3c80
Gerrit-Change-Number: 15985
Gerrit-PatchSet: 2
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: Fri, 06 Aug 2021 23:09:19 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Joshua Colp <jcolp at sangoma.com>
Comment-In-Reply-To: N A <mail at interlinked.x10host.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20210806/ad2aa912/attachment.html>