[Asterisk-code-review] app_queue: Add QueueWithdrawCaller AMI action (asterisk[16])

[Kfir Itzhak]

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

Change subject: app_queue: Add QueueWithdrawCaller AMI action
......................................................................


Patch Set 4:

(7 comments)

Commit Message:

https://gerrit.asterisk.org/c/asterisk/+/18008/comment/be2bc8d9_778bf31a 
PS2, Line 14: It is a best-effort request. It works by signaling the
> Can you clarify what best-effort really means? Is it guaranteed, or can it not happen?
It may not happen, because its asynchronous. 
It may not happen in these cases:
1) If the caller is now listening to an annoucement, such as position, it will only leave after this announcement.
2) If the caller is now inside a breakout menu, it will only leave after exiting this menu.
3) If the caller is already ringing at an extension, it will wait only leave after the ringing ends and the call wasn't answered.

These are limitations because i have no control over the caller's channel, it runs in a different thread. I can only signal it to leave the queue.
I could perhaps make the function block, but this will block the AMI connection until #1, #2 or #3 are done.
I am not concerned about #1 and #2. I find #3 a little more concerning, but i am not sure how to change this behaviour safely.
So this is why i wrote that its a best-effort request.

It is possible to extend the behaviour in the future with a parameter, perhaps something like Async: False, similiar to Originate.


Patchset:

PS4: 
Thanks for the comments. All (hopefully) resolved :-)


File apps/app_queue.c:

https://gerrit.asterisk.org/c/asterisk/+/18008/comment/d3a63896_6a090d4f 
PS2, Line 301: 				</variable>
> The QUEUE_WITHDRAW_INFO dialplan variable should be documented here.
Ack


https://gerrit.asterisk.org/c/asterisk/+/18008/comment/6511eee7_e803cf0c 
PS2, Line 1713: 	char* withdraw_info;                   /*!< Optional info passed by the caller of QueueWithdrawCaller */
> char *withdraw_info;
Ack


https://gerrit.asterisk.org/c/asterisk/+/18008/comment/7e9c4df3_309ccd51 
PS2, Line 7645: 		if (strcmp(ast_channel_name(qe->chan), caller) == 0) {
> Please review the coding guidelines[1]. […]
Ack


https://gerrit.asterisk.org/c/asterisk/+/18008/comment/1c65d282_0bff2ed2 
PS2, Line 7648: 			if(withdraw_info && !qe->withdraw_info) {
> In what case would withdraw_info be set already?
In case another request was sent already.
In such case, i thought that its better not to overwrite the existing withdraw info, because perhaps the call is about to leave already due to the first request.
If we decide to allow overwriting the withdraw_info, we must free the existing first.


https://gerrit.asterisk.org/c/asterisk/+/18008/comment/e8de5dc4_bf63f4f8 
PS2, Line 10805: 	switch (request_withdraw_caller_from_queue(queuename, caller, withdraw_info)) {
> I think from a user perspective this should tell the user if the channel is already in the process o […]
Ok, i agree.



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

Gerrit-Project: asterisk
Gerrit-Branch: 16
Gerrit-Change-Id: Ic15aa238e23b2884abdcaadff2fda7679e29b7ec
Gerrit-Change-Number: 18008
Gerrit-PatchSet: 4
Gerrit-Owner: Kfir Itzhak <mastertheknife at gmail.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Attention: Joshua Colp <jcolp at sangoma.com>
Gerrit-Comment-Date: Sun, 13 Feb 2022 12:22:07 +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/20220213/fc573eef/attachment-0001.html>