<p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p>Patch set 3:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p><a href="https://gerrit.asterisk.org/10523">View Change</a></p><p>4 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10523/3/apps/app_queue.c">File apps/app_queue.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/10523/3/apps/app_queue.c@6190">Patch Set #3, Line 6190:</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;"> if (caller_snapshot && member_snapshot) {<br> send_agent_complete(queue_data->queue->name, caller_snapshot, member_snapshot, queue_data->member,<br> queue_data->holdstart, queue_data->starttime, TRANSFER);<br> } else {<br> ast_log(LOG_WARNING, "Could not retrieve caller_snapshot or member_snapshot; not sending AgentComplete event\n");<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">See comment below about this in the handle_attended_transfer routine</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10523/3/apps/app_queue.c@6253">Patch Set #3, Line 6253:</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;"> if (caller_snapshot) {<br> log_attended_transfer(queue_data, caller_snapshot, atxfer_msg);<br> } else {<br> ast_log(LOG_WARNING, "Could not retrieve caller_snapshot; not logging attended transfer\n");<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10523/3/apps/app_queue.c@6259">Patch Set #3, Line 6259:</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;"> if (caller_snapshot && member_snapshot) {<br> send_agent_complete(queue_data->queue->name, caller_snapshot, member_snapshot, queue_data->member,<br> queue_data->holdstart, queue_data->starttime, TRANSFER);<br> } else {<br> ast_log(LOG_WARNING, "Could not retrieve caller_snapshot or member_snapshot; not sending AgentComplete event\n");<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.<br>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.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10523/3/apps/app_queue.c@6465">Patch Set #3, Line 6465:</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;"> if (caller_snapshot && member_snapshot) {<br> send_agent_complete(queue_data->queue->name, caller_snapshot, member_snapshot, queue_data->member,<br> queue_data->holdstart, queue_data->starttime, reason);<br> } else {<br> ast_log(LOG_WARNING, "Could not retrieve caller_snapshot or member_snapshot; not sending AgentComplete event\n");<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">see comment above in handle_attended_transfer about potentially moving this functionality into another function.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/10523">change 10523</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/10523"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 16 </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I37334ea184ebb56e54471496b82937d4927815a0 </div>
<div style="display:none"> Gerrit-Change-Number: 10523 </div>
<div style="display:none"> Gerrit-PatchSet: 3 </div>
<div style="display:none"> Gerrit-Owner: lvl <digium@lvlconsultancy.nl> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 (1000185) </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Sean Bright <sean.bright@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: lvl <digium@lvlconsultancy.nl> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 21 Nov 2018 00:14:19 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>