<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/4135/">https://reviewboard.asterisk.org/r/4135/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On October 31st, 2014, 12:39 p.m. CDT, <b>rmudgett</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://reviewboard.asterisk.org/r/4135/diff/2/?file=68675#file68675line4253" style="color: black; font-weight: bold; text-decoration: underline;">/branches/12/main/bridge.c</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">enum ast_transfer_result ast_bridge_transfer_blind(int is_external,</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">4243</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="n">transfer_result</span> <span class="o">=</span> <span class="n">try_parking</span><span class="p">(</span><span class="n">transferer</span><span class="p">,</span> <span class="n">context</span><span class="p">,</span> <span class="n">exten</span><span class="p">,</span> <span class="n">new_channel_cb</span><span class="p">,</span> <span class="n">user_data_wrapper</span><span class="p">);</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">4247</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="n">transfer_result</span> <span class="o">=</span> <span class="n">try_parking</span><span class="p">(</span><span class="n">transferer</span><span class="p">,</span> <span class="n">context</span><span class="p">,</span> <span class="n">exten</span><span class="p">,</span> <span class="n">new_channel_cb</span><span class="p">,</span> <span class="n">user_data_wrapper</span><span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I have a feeling that the race also affects parking here. If so then holding the bridge lock is not a viable solution.</pre>
</blockquote>
<p>On October 31st, 2014, 3:21 p.m. CDT, <b>Mark Michelson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I took a gander through the parking code and didn't find anywhere where the blind transfer operation was being queued onto a bridge channel. Did you have a specific spot you had found in the parking code that looked suspicious?</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">At the end of res/parking/parking_bridge_features.c:parking_blind_transfer_park() an action is written to the bridge for the peer to move itself to the parking/holding bridge. If this happens before the transfer is published you have the same race condition.</pre>
<br />
<p>- rmudgett</p>
<br />
<p>On October 31st, 2014, 10:58 a.m. CDT, Mark Michelson wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By Mark Michelson.</div>
<p style="color: grey;"><i>Updated Oct. 31, 2014, 10:58 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
Asterisk
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">During blind transfer testing, it was noticed that tests were failing occasionally because the ARI blind transfer event was not being sent. After investigating, I detected a race condition in the blind transfer code. When blind transferring a single channel, the actual transfer operation (i.e. removing the transferee from the bridge and directing them to the proper dialplan location) is queued onto the transferee bridge channel. After queuing the transfer operation, the blind transfer Stasis message is published. At the time of publication, snapshots of the channels and bridge involved are created. The ARI subscriber to the blind transfer Stasis message then attempts to determine if the bridge or any of the involved channels are subscribed to by ARI applications. If so, then the blind transfer message is sent to the applications. The way that the ARI blind transfer message handler works is to first see if the transferer channel is subscribed to. If not, then iterate over all the channel IDs in the bridge snapshot and determine if any of those are subscribed to. In the test we were running, the lone transferee channel was subscribed to, so an ARI event should have been sent to our application. Occasionally, though, the bridge snapshot did not have any channels IDs on it at all. Why?
The problem is that since the blind transfer operation is handled by a separate thread, it is possible that the transfer will have completed and the channels removed from the bridge before we publish the blind transfer Stasis message. Since the blind transfer has completed, the bridge on which the transfer occurred no longer has any channels on it, so the resulting bridge snapshot has no channels on it. Through investigation of the code, I found that attended transfers can have this issue too for the case where a transferee is transferred to an application.
To fix this problem, I thought of four possible fixes:
1) Let the thread that actually performs the blind transfer publish the Stasis message.
2) Get the bridge snapshot before queuing the blind transfer operation.
3) Publish the blind transfer Stasis message before queuing the blind transfer operation.
4) Hold the bridge lock while queuing the blind transfer operation and publishing the blind transfer Stasis message.
Option 1 is clearly the "best" option, but it also is made nearly impossible due to the way that bridge channel operations are queued. Bridge channel operations use frames, which require that their payload be copyable using memcpy(). Including any sort of pointer is a no-no. So I would be forced to come up with some inane method of representing multiple channels and bridges in a frame in order to do this.
Option 2 is slightly more workable. Currently, there are functions to publish blind and attended transfers that require bridges and channels, not snapshots. Changing the API to accommodate snapshots is possible, but it is more widespread than I would like, and it changes the API. It also runs the slight risk of publishing "stale" data, though that is not likely.
Option 3 is easiest to implement, but it runs the (very slight) risk that we could publish that a blind transfer happened successfully when in fact the attempt to queue the blind transfer operation failed. I almost went with this, but I felt like the possibility of lying in our events was a bad thing to do.
Option 4 is my least favorite, but within a release felt like the most viable method. The bridge is already locked while publishing the transfer messages, and I didn't see any harm in having the bridge locked while queuing the transfer operation either. By holding the bridge lock the entire time, we prevent the bridge state from changing out from under us while we publish our Stasis message. This means that the bridge will stay stable until we finally unlock after publishing the Stasis message.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I have re-run the blind transfer tests that had occasionally failed, and I do not see failures. More importantly, the tests still function properly and the change to locking has not introduced any noticeable bad effects.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/branches/12/main/bridge.c <span style="color: grey">(426802)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/4135/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>