<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/3099/">https://reviewboard.asterisk.org/r/3099/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On January 8th, 2014, 1:56 p.m. CST, <b>opticron</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/3099/diff/1/?file=51113#file51113line593" style="color: black; font-weight: bold; text-decoration: underline;">/branches/12/main/stasis.c</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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; ">static int topic_remove_subscription(struct stasis_topic *topic, struct stasis_subscription *sub)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">593</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="k">if</span> <span class="p">(</span><span class="n">ast_taskprocessor_push_local</span><span class="p">(</span><span class="n">sub</span><span class="o">-></span><span class="n">mailbox</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;">When synchronous dispatch is required, why not just dispatch directly as if there were no mailbox?</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;">It's a good notion, but that wouldn't actually work here.
In the case of synchronizing on delivery of the message over the task processor, we know that the subscription has a mailbox. Since it has a mailbox, it has a taskprocessor that may have many messages enqueued on it. Having a synchronous message published to the subscriber bypassing the mailbox would have that message arrive before messages that were published already to the subscriber. Messages delivered to a subscription must arrive in the order that they were published. Otherwise, the message processing on the subscriber's end becomes a chaotic wreck, and it would be up to the subscriber to re-order all messages.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On January 8th, 2014, 1:56 p.m. CST, <b>opticron</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/3099/diff/1/?file=51113#file51113line620" style="color: black; font-weight: bold; text-decoration: underline;">/branches/12/main/stasis.c</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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; ">static int topic_remove_subscription(struct stasis_topic *topic, struct stasis_subscription *sub)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">620</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="k">static</span> <span class="kt">void</span> <span class="nf">stasis_publish_msg</span><span class="p">(</span><span class="k">struct</span> <span class="n">stasis_topic</span> <span class="o">*</span><span class="n">topic</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;">This should not have the stasis_ prefix if it isn't available outside this translation unit.</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;">Fixed</pre>
<br />
<p>- Matt</p>
<br />
<p>On December 31st, 2013, 1:24 p.m. CST, Matt Jordan 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 and David Lee.</div>
<div>By Matt Jordan.</div>
<p style="color: grey;"><i>Updated Dec. 31, 2013, 1:24 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-22884">ASTERISK-22884</a>
</div>
<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;">In https://reviewboard.asterisk.org/r/3057/, applications and functions that manipulate CDRs were made to interact over Stasis. This was done to synchronize manipulations of CDRs from the dialplan with the updates the engine itself receives over the message bus.
This change rested on a faulty premise: that messages published to the CDR topic or to a topic that forwards to the CDR topic are synchronized with the messages handled by the CDR topic subscription in the CDR engine. This is not the case. There is no ordering guaranteed for two messages published to the same topic; ordering is only guaranteed if a message is published to the same subscriber.
Note that the patch actually still fixed a bunch of test failures by virtue of publishing the messages in the first place to the channel topic; however, the hangup handler tests - which use the CDR function to read a value from the engine - did still fail after this patch. Reading data requires an explicit synchronization with the engine.
In order to fix this problem, this patch does the following:
(1) It provides a new way to publish a message. It is now possible to publish a message and specify which stasis_subscription you want to synchronize on. A message published in such a fashion is still published to all subscribers of the topic (which is needed; you don't know who all wants your message), but the call will block until the specified subscriber handles the message.
(2) It updates the CDR engine to exposed its message router (which contains the subscription). CDR related applications now synchronize on the message router when publishing information. As a result, the CDR topic/router now persist across CDR engine disablings; the only thing that is removed when the engine is disabled are the forwarding subscriptions. This keeps a lot of quirky lifetime issues from becoming serious problems, while still maintaining performance if the CDR engine is disabled.</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;">Unit test for publishing synchronously was added. It passes.
All stasis and CDR unit tests pass.
All CDR and hangup handler test in the test suite now pass. Previously, the hangup handler tests would fail due to getting wrong data back from the CDR engine when reading a value using the CDR function.</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/tests/test_stasis.c <span style="color: grey">(404591)</span></li>
<li>/branches/12/main/stasis_message_router.c <span style="color: grey">(404591)</span></li>
<li>/branches/12/main/stasis.c <span style="color: grey">(404591)</span></li>
<li>/branches/12/main/cdr.c <span style="color: grey">(404591)</span></li>
<li>/branches/12/include/asterisk/stasis_message_router.h <span style="color: grey">(404591)</span></li>
<li>/branches/12/include/asterisk/stasis.h <span style="color: grey">(404591)</span></li>
<li>/branches/12/include/asterisk/cdr.h <span style="color: grey">(404591)</span></li>
<li>/branches/12/funcs/func_cdr.c <span style="color: grey">(404591)</span></li>
<li>/branches/12/apps/app_forkcdr.c <span style="color: grey">(404591)</span></li>
<li>/branches/12/apps/app_cdr.c <span style="color: grey">(404591)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/3099/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>