<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/3382/">https://reviewboard.asterisk.org/r/3382/</a>
</td>
</tr>
</table>
<br />
<div>
<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/3382/diff/3/?file=56347#file56347line2536" style="color: black; font-weight: bold; text-decoration: underline;">branches/12/main/astobj2.c</a>
<span style="font-weight: normal;">
(Diff revision 3)
</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 struct hash_bucket_node *hash_ao2_find_first(struct ao2_container_hash *self, enum search_flags flags, void *arg, struct hash_traversal_state *state)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">2334</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="k">if</span> <span class="p">(</span><span class="o">!</span><span class="n">node</span><span class="o">-></span><span class="n">common</span><span class="p">.</span><span class="n">obj</span><span class="p">)</span> <span class="p">{</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">2406</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="k">if</span> <span class="p">(</span><span class="o">!</span><span class="n">node</span><span class="o">-></span><span class="n">common</span><span class="p">.</span><span class="n">obj</span> <span class="o">||</span> <span class="n">INTERNAL_OBJ_DESTROYING</span><span class="p">(</span><span class="n">node</span><span class="o">-></span><span class="n">common</span><span class="p">.</span><span class="n">obj</span><span class="p">))</span> <span class="p">{</span></pre></td>
</tr>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">2335</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="cm">/* Node is empty */</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">2407</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="cm">/* Node is empty <span class="hl">or being destroyed </span>*/</span></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">2336</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="k">continue</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">2408</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="k">continue</span><span class="p">;</span></pre></td>
</tr>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">2337</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="tb"> </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">2409</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="p">}</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Another thread can remove the last reference immediately after this thread finds that node->common.obj is not destroyed, allowing return of an object that is being (or has already been) destroyed.
Any time another thread does ao2_ref(obj, -1); on the object we're trying to retreive we are subject to a race. A big problem is that ao2_ref requires that the caller owns a reference to the object, and no other thread will release that reference. I don't think it's possible to safely change this requirement.</pre>
</div>
<br />
<p>- Corey Farrell</p>
<br />
<p>On March 24th, 2014, 1:17 p.m. EDT, 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, Joshua Colp and rmudgett.</div>
<div>By George Joseph.</div>
<p style="color: grey;"><i>Updated March 24, 2014, 1:17 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 is a Request For Comments patch that adds weak reference capability to astobj2 containers.
Current (Strong-ref) behavior: A container (list, hash or rbtree) increments an object's ref count when linked and decrements an object's ref count when unlinked. Therefore the object can never be destroyed while it is an entry in a container unless someone accidentally decrements the object's ref count too many times. In this case, the object is invalid but the container doesn't know it.
Weak-ref behavior: A container (list, hash or rbtree) allocated with the option AO2_CONTAINER_ALLOC_OPT_WEAK_REF neither increments an object's ref count when linked nor decrements an object's ref count when unlinked. If the object's ref count can therefore validly reach 0 in which case the object is automatically and cleanly removed from any weak-ref container it may be in.
Use case: The possible need for weak-ref containers came up during the development of the Sorcery registry. The first call to sorcery_open from a module should create a new sorcery instance and subsequent calls from that same module should use that instance. The last call to close should free that instance. With a strong-ref container however, the container itself always has a a reference to the instance so it doesn't get destroyed without special code to check the ref count on each call to close.
Implementation: astobj2.priv_data now has a linked list that contains the weak-ref container nodes that reference the object. When an object is added to a weak-ref container, the container node is added to the list instead of the node incrementing the object's ref count. Similarly, when an object is removed from a weak-ref container the node is removed from the linked list instead of the object's ref count being decremented. If an object's ref count reaches 0 the object's linked list is traversed and the corresponding nodes are cleared effectively removing the object from the container. NOTE: An object's ref count is still incremented as the result of a retrieval (find, iterate, etc) from a weak-ref container.
Backwards compatibility: Unless the AO2_CONTAINER_ALLOC_OPT_WEAK_REF flag was set on container allocation, all container operations remain exactly as they are today and nothing prevents an object from being a member of both strong and weak ref containers at the same time.
Performance implications: Due to code reorganization, the performance of strong-ref containers is actually microscopically BETTER than the unmodified code and the performance of weak-ref containers is even better than that. In other words, the performance of the default behavior was not penalized by the addition of the new feature.
Code reorganization. I moved all of the structure definitions in astobj2.c to astobj2_private.h. This makes astobj2.c much easier to read and debug. I was also able to push down some implementation specific code to the hash and rbtree functions and pull up some duplicated code from the hash and rbtree functions.
Patch notes: The patch file itself might be a little hard to read because of the reorganization so applying the patch is your best bet for detailed review. The patch will apply cleanly to both branches/12 and trunk. Also, the patch disables AO2_DEBUG and AST_DEVMODE in astobj2 so that performance can be tested while still keeping the test framework. The final patch will remove the disables.
</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 of the existing unit tests in test_astobj2 pass including the thrash test.
I added 7 additional unit tests specifically for the weak-ref implementation including a performance comparison test that compares both strong and weak ref implementations. A thrash test was also added for weak-ref.
</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_astobj2_thrash.c <span style="color: grey">(411013)</span></li>
<li>branches/12/tests/test_astobj2.c <span style="color: grey">(411013)</span></li>
<li>branches/12/main/astobj2.c <span style="color: grey">(411013)</span></li>
<li>branches/12/include/asterisk/astobj2.h <span style="color: grey">(411013)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/3382/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>