[Asterisk-code-review] app queue: Revert broken queue channel reference patch (asterisk[16])

[Kevin Harwell]

Kevin Harwell has posted comments on this change. ( https://gerrit.asterisk.org/10523 )

Change subject: app_queue: Revert broken queue channel reference patch
......................................................................


Patch Set 3: Code-Review-1

(4 comments)

This is a very tough call. I think at this time I lean toward moving forward with this patch (see comments about potential modifications). The ghost calls mentioned in ASTERISK-25844 will disappear, but there is still a leak present. However that leak is present either way (and should be fixed at some point). Without the added channel refs at least the channels aren't held.

It appears to even with the original patch something is still getting by and the caller_snapshot can be NULL. After looking back at the code I'm not entirely sure how this could be happening at this time. This patch should at least stop the crashes and it appears that at least in the queue_log case it doesn't even need the snapshot as the data was already previously stored.

However, I agree too with Josh that all these issues should remain open and I'd put comments on each of them linking this patch and describing how this is just a stop gap.

https://gerrit.asterisk.org/#/c/10523/3/apps/app_queue.c
File apps/app_queue.c:

https://gerrit.asterisk.org/#/c/10523/3/apps/app_queue.c@6190
PS3, Line 6190: 	if (caller_snapshot && member_snapshot) {
              : 		send_agent_complete(queue_data->queue->name, caller_snapshot, member_snapshot, queue_data->member,
              : 			queue_data->holdstart, queue_data->starttime, TRANSFER);
              : 	} else {
              : 		ast_log(LOG_WARNING, "Could not retrieve caller_snapshot or member_snapshot; not sending AgentComplete event\n");
              : 	}
See comment below about this in the handle_attended_transfer routine


https://gerrit.asterisk.org/#/c/10523/3/apps/app_queue.c@6253
PS3, Line 6253: 	if (caller_snapshot) {
              : 		log_attended_transfer(queue_data, caller_snapshot, atxfer_msg);
              : 	} else {
              : 		ast_log(LOG_WARNING, "Could not retrieve caller_snapshot; not logging attended transfer\n");
              : 	}
The only thing log_attended_transfer uses the given snapshot for is a reference to the uniqueid. I think instead of printing a message here you could just not pass caller_snapshot, and in log_attended_transfer use queue_data->caller_uniqueid (similar to how you did it in handle_blind_transfer). That way the data is still logged.


https://gerrit.asterisk.org/#/c/10523/3/apps/app_queue.c@6259
PS3, Line 6259: 	if (caller_snapshot && member_snapshot) {
              : 		send_agent_complete(queue_data->queue->name, caller_snapshot, member_snapshot, queue_data->member,
              : 			queue_data->holdstart, queue_data->starttime, TRANSFER);
              : 	} else {
              : 		ast_log(LOG_WARNING, "Could not retrieve caller_snapshot or member_snapshot; not sending AgentComplete event\n");
              : 	}
Here if you follow the call into send_agent_complete it simply passes the snapshots on to queue_publish_multi_channel_snapshot_blob. At this point one option would be to put a NULL check for the caller_snapshot (like is being done for agent_snapshot) in this function. The message is still published but potentially missing the call and/or agent payload info, but at the least the message is still raised. Not sure how that might affect subscribers to that message type.
The other option is to do basically what you are doing here, but in queue_publish_multi_channel_snapshot_blob. If both are NULL then log the warning there and return without publishing.


https://gerrit.asterisk.org/#/c/10523/3/apps/app_queue.c@6465
PS3, Line 6465: 	if (caller_snapshot && member_snapshot) {
              : 		send_agent_complete(queue_data->queue->name, caller_snapshot, member_snapshot, queue_data->member,
              : 			queue_data->holdstart, queue_data->starttime, reason);
              : 	} else {
              : 		ast_log(LOG_WARNING, "Could not retrieve caller_snapshot or member_snapshot; not sending AgentComplete event\n");
              : 	}
see comment above in handle_attended_transfer about potentially moving this functionality into another function.



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

Gerrit-Project: asterisk
Gerrit-Branch: 16
Gerrit-MessageType: comment
Gerrit-Change-Id: I37334ea184ebb56e54471496b82937d4927815a0
Gerrit-Change-Number: 10523
Gerrit-PatchSet: 3
Gerrit-Owner: lvl <digium at lvlconsultancy.nl>
Gerrit-Reviewer: Jenkins2 (1000185)
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Sean Bright <sean.bright at gmail.com>
Gerrit-Reviewer: lvl <digium at lvlconsultancy.nl>
Gerrit-Comment-Date: Wed, 21 Nov 2018 00:14:19 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20181120/b6593dd3/attachment-0001.html>