<div dir="ltr"><div><div><div><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Dec 16, 2015 at 3:18 AM, Tomec Martin <span dir="ltr"><<a href="mailto:tomec@ipex.cz" target="_blank">tomec@ipex.cz</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div link="blue" vlink="purple" lang="CS">
<div>
<p class="MsoNormal"><span lang="EN-US">Hi all,<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US"><u></u> <u></u></span></p>
<p class="MsoNormal"><span lang="EN-US">I am solving issue ASTERISK-19820 and want to write patch, but I am not sure if I am on good way. Let me explain the issue:<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US">When agent in queue hangs up, his status is updated immediately, but the queue member lastcall time is updated after a while (after some logging etc.) in function update_queue. So when another caller wants to ring this
agent, it gets new member status (not_inuse) and old queue member data (with old lastcall time). So wrapuptime is compared to wrong (old) lastcall time.<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US"><u></u> <u></u></span></p>
<p class="MsoNormal"><span lang="EN-US">My supposed solution is:<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US">Add new variable „is_in_call“ to struct member<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US">Set „is_in_call“ to true, when the call starts.<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US">Set „is_in_call“ to false on hangup callback (in function update_queue).<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US">Together with wrapuptime check also if „is_in_call“ is set to true.<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US"><u></u> <u></u></span></p>
<p class="MsoNormal"><span lang="EN-US">I don’t know how to determine call start and I am not sure if the variable should be named „is_in_call“. Any opinions are welcome.<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US"><u></u> <u></u></span></p>
<p class="MsoNormal"><span lang="EN-US">Thanks,<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US">Martin Tomec<u></u><u></u></span></p>
<p class="MsoNormal"><br clear="all"></p></div></div></blockquote></div><br></div><div class="gmail_extra">Hi Martin:<br><br></div><div class="gmail_extra">Most of my thoughts on this come from Asterisk 13. Looking at the issue, it appears as if the situation hasn't changed between when that issue was filed and now, so hopefully this line of thinking is applicable to whatever version you're looking at.<br></div><div class="gmail_extra"><br></div><div class="gmail_extra">I can definitely see how this could be a bit racey. We essentially have two things competing here:<br></div><div class="gmail_extra"> * The updating of the queue member status in update_status. This is, interestingly enough, updated via a device state change or extension state change. Even all the way back in 1.8 - and earlier - this would be handled by some kind of asynchronous message mechanism, which would certainly be happening on a different thread of execution than the channel threads sitting in the queue.<br></div><div class="gmail_extra"> * The update of the 'queue' - which is really updating the member's state in the queue, followed by updating various statistics of the queue - happens in 13 when we detect one of three things happening:<br></div><div class="gmail_extra"> (1) A blind transfer away from the member<br></div><div class="gmail_extra"> (2) An attended transfer away from the member<br></div><div class="gmail_extra"> (3) A hangup of the member<br></div><div class="gmail_extra"> In all of these cases, these again are handled asynchronously off the message bus. That wouldn't be the case in 11 and earlier - in those versions, we'd be looking directly at either a masquerade occurring or soft hangup flags on the member channel. Either way, it's certainly out of band with the calls to update_status - and it is the same again in 13.</div><br></div>I've tried to think of another solution other than what you're proposing. Essentially, we'd ideally like to rely on status to know if the member is on a call or not; however, given the race condition occurring between the changing of the member state (which should tell us that) and the "clean up of the member in the queue" logic that is called via update_queue, I don't think that's quite possible.<br><br></div>I don't think we can also easily combine update_queue with update_status. update_status is called whenever a device/extension state changes, and applies in situations other than just termination of a call. update_queue, on the other hand, only happens on call end, and has certain logic that it needs.<br><br></div>So: adding something to track the boolean state of whether or not a member is on a call or not is probably the best approach.<br><div><div><div><div><div><div class="gmail_extra"><br></div><div class="gmail_extra">As a cautionary point, I would be careful to test this logic here:<br><br>static int is_member_available(struct call_queue *q, struct member *mem)<br>{<br> int available = 0;<br><br> switch (mem->status) {<br> case AST_DEVICE_INVALID:<br> case AST_DEVICE_UNAVAILABLE:<br> break;<br> case AST_DEVICE_INUSE:<br> case AST_DEVICE_BUSY:<br> case AST_DEVICE_RINGING:<br> case AST_DEVICE_RINGINUSE:<br> case AST_DEVICE_ONHOLD:<br> if (!mem->ringinuse) {<br> break;<br> }<br> /* else fall through */<br> case AST_DEVICE_NOT_INUSE:<br> case AST_DEVICE_UNKNOWN:<br> if (!mem->paused) {<br> available = 1;<br> }<br> break;<br> }<br><br> /* Let wrapuptimes override device state availability */<br> if (mem->lastcall && q->wrapuptime && (time(NULL) - q->wrapuptime < mem->lastcall)) {<br> available = 0;<br> }<br> return available;<br>}<br><br></div><div class="gmail_extra">As it would be prone to the race condition you're describing.<br></div><div class="gmail_extra"><br>-- <br><div class="gmail_signature"><div dir="ltr"><div><div dir="ltr"><div>Matthew Jordan<br></div><div>Digium, Inc. | Director of Technology<br></div><div>445 Jan Davis Drive NW - Huntsville, AL 35806 - USA</div><div>Check us out at: <a href="http://digium.com" target="_blank">http://digium.com</a> & <a href="http://asterisk.org" target="_blank">http://asterisk.org</a></div></div></div></div></div>
</div></div></div></div></div></div></div>