[prev in list] [next in list] [prev in thread] [next in thread]
List: asterisk-dev
Subject: Re: [asterisk-dev] [Code Review] 2419: stasis: Fixed message ordering issues when forwarding
From: "opticron" <reviewboard () asterisk ! org>
Date: 2013-03-31 1:59:46
Message-ID: 20130331015946.28438.79446 () hotblack ! digium ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/2419/#review8155
-----------------------------------------------------------
Ship it!
Guaranteed ordering is definitely a good thing.
- opticron
On March 29, 2013, 11:40 p.m., David Lee wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2419/
> -----------------------------------------------------------
>
> (Updated March 29, 2013, 11:40 p.m.)
>
>
> Review request for Asterisk Developers, Matt Jordan and opticron.
>
>
> Repository: Asterisk
>
>
> Description
> -------
>
> This patch fixes an issue of message ordering that occurs when
> multiple topics are forwarded to an aggregator topic (such as
> ast_channel_topic_all()).
>
> It is (very reasonably) expected that the rules governing message
> dispatch order still apply, so long as the messages start from the
> same thread, and are received by the same subscription. Because the
> existing code had an additional layer of dispatching via the Stasis
> thread pool for forwards, those promises couldn't be kept.
>
> Forwarding subscriptions no longer have their own mailbox, and now
> dispatch directly from the forwarding topic's stasis_publish()
> call. This means that the topic's lock is held for the duration of not
> only a message's dispatch, but the dispatch of all the forwards. This
> shouldn't be a problem right now, but if an aggregator topic had many
> subscribers, it could become a problem. But I figure we can write more
> clever code when the time comes, if necessary.
>
>
> Diffs
> -----
>
> /team/dlee/stasis-interleaving/tests/test_stasis.c 384385
> /team/dlee/stasis-interleaving/main/stasis.c 384385
>
> Diff: https://reviewboard.asterisk.org/r/2419/diff/
>
>
> Testing
> -------
>
> Unit test that demonstrates the expected behavior. It fails without the patch; passes with it.
>
>
> Thanks,
>
> David Lee
>
>
[Attachment #5 (text/html)]
<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/2419/">https://reviewboard.asterisk.org/r/2419/</a>
</td>
</tr>
</table>
<br />
<p>Ship it!</p>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Guaranteed ordering is \
definitely a good thing.</pre> <br />
<p>- opticron</p>
<br />
<p>On March 29th, 2013, 11:40 p.m. CDT, David Lee 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.png'); \
background-position: left top; background-repeat: repeat-x; border: 1px black \
solid;"> <tr>
<td>
<div>Review request for Asterisk Developers, Matt Jordan and opticron.</div>
<div>By David Lee.</div>
<p style="color: grey;"><i>Updated March 29, 2013, 11:40 p.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;">This patch fixes an issue of message ordering that occurs when multiple \
topics are forwarded to an aggregator topic (such as ast_channel_topic_all()).
It is (very reasonably) expected that the rules governing message
dispatch order still apply, so long as the messages start from the
same thread, and are received by the same subscription. Because the
existing code had an additional layer of dispatching via the Stasis
thread pool for forwards, those promises couldn't be kept.
Forwarding subscriptions no longer have their own mailbox, and now
dispatch directly from the forwarding topic's stasis_publish()
call. This means that the topic's lock is held for the duration of not
only a message's dispatch, but the dispatch of all the forwards. This
shouldn't be a problem right now, but if an aggregator topic had many
subscribers, it could become a problem. But I figure we can write more
clever code when the time comes, if necessary.</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 that demonstrates the expected behavior. It fails without the \
patch; passes with it.</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>/team/dlee/stasis-interleaving/tests/test_stasis.c <span style="color: \
grey">(384385)</span></li>
<li>/team/dlee/stasis-interleaving/main/stasis.c <span style="color: \
grey">(384385)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/2419/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
--
_____________________________________________________________________
-- Bandwidth and Colocation Provided by http://www.api-digital.com --
asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
http://lists.digium.com/mailman/listinfo/asterisk-dev
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic