<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/4572/">https://reviewboard.asterisk.org/r/4572/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On April 1st, 2015, 6:21 p.m. MDT, <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;">As a concept, this feels like a suitable stop-gap solution. Better solutions would be alterations to PJSIP itself. Either
1) Make pjsip_endpt_send_request() not ignore the timeout parameter passed to it.
2) Make pjsip transaction timers configurable per transaction.
The solution provided here has the issue that it has an Asterisk scheduler and a PJLIB timer competing with each other, when it would be less wasteful to have the PJLIB timer do everything. Because of the competing timers, it's possible for the transaction state callback to be called into from multiple threads at the same time. If the transaction timers could be altered per transaction, then there would only ever be a single timer running, and the possibility of races is eliminated.</pre>
</blockquote>
</blockquote>
<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'm going to work on a pjproject patch as well but it'll probably take a while to get mainlined. The sched timer itself doesn't run the callback when it triggers, it only cancels the transaction and pjsip runs the callback so the callback should never be called by multiple threads.
</pre>
<br />
<p>- George</p>
<br />
<p>On March 30th, 2015, 11:57 p.m. MDT, George Joseph 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 George Joseph.</div>
<p style="color: grey;"><i>Updated March 30, 2015, 11:57 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-24863">ASTERISK-24863</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;">This review is based on the discussion here
http://lists.digium.com/pipermail/asterisk-dev/2015-March/073921.html
-------------------------------------
Right now when a contact is qualified, it's immediately marked as
UNAVAILABLE. If a response to the OPTIONS message comes back, it's
marked as AVAILABLE again and the round trip time calculated. The
status flapping even in normal operation makes generating events
tricky because the subscriber will see up/down events even if the
contact is really AVAILABLE. Now the pjsip transaction will
eventually time out at an unconfigurable 32 seconds and call the
callback but 32 seconds is a long time. This is also an issue if the
qualify_frequency is less than 32 seconds since they'll be multiple
pjsip transactions in progress for the same contact.
<snip>
So, I'm proposing pulling libpjsip/pjsip_endpt_send_request up into
res_pjsip. It's 2 short function and a pjsip_module structure with
no access to any private pjsip stuff. Now we'll have the ability to
terminate the transaction AND it seems that there's a timeout member
of pjsip_transaction which I'm hoping (but haven't tested) will
eliminate the need to add timeout processing in pjsip_options.
The result of all of this is that we'll be able to generate events for
contact status (ASTERISK-24863) which in turn will help is provide
functionality like marking an endpoint unavailable when all of its
contacts are unavailable.
-------------------------------------
Both Josh and Mark had comments in the thread so this is an RFC ONLY review.
It covers the pull up of libpjsip/pjsip_endpt_send_request but I've also added the
use case which is creating and processing a new aor/contact option called
'qualify_timeout'. I'll split the final submission into 2 pieces.
</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;">All existing pjsip related tests complete. I'll also have to add event generation on contact status change and the tests based on 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>branches/13/res/res_pjsip/pjsip_options.c <span style="color: grey">(433794)</span></li>
<li>branches/13/res/res_pjsip/location.c <span style="color: grey">(433794)</span></li>
<li>branches/13/res/res_pjsip.c <span style="color: grey">(433794)</span></li>
<li>branches/13/include/asterisk/res_pjsip.h <span style="color: grey">(433794)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/4572/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>