<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/3716/">https://reviewboard.asterisk.org/r/3716/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 24th, 2014, 4:01 p.m. CDT, <b>rmudgett</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;">You are affecting the lifetime of the continer node object without protecting it. It can now potentially be used outside the lifetime of its container as part of the held object. The container node and the continer ponter in the container node would be stale. This can happen when the container is being destroyed at the same time as the object within the container is being destroyed. You would need to give the object a reference to the container node and the container itself. The reference to the container prevents the nasty destruction race collision. Yes, all objects in the weak container would need to die before the container could self destruct or the weak container can be explicitly emptied to get rid of it earlier. However, since they are expected to be used as global containers that is not a problem at shutdown.
It would be much simpler if the held object just contained a list of the weak containers with a reference instead of the container's node object. The ao2 object's clean up could simply use ao2_unlink() to remove itself from the weak container. The use of the container node to pull double duty adds just too much coupling and complexity.
Weak containers can only return an object after successfully getting its reference (ao2_ref(obj, +1) returning a non-negative value). If it fails internal_ao2_traverse() must go on to the next object.</pre>
</blockquote>
<p>On July 25th, 2014, 11:14 a.m. CDT, <b>George Joseph</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;">An object can't use a list of containers as this would mean the container could only be in one object's list at a time. I'd have to use a container of containers which would be significant overhead. That's why I went with node since a node can only be associated with one object. Having a reference to the container makes sense though. I think Corey suggested that a while back.</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 think you are making an invalid assumption about ao2 containers. An ao2 container can hold the same object more than once. The AO2_CONTAINER_ALLOC_OPT_DUPS_xxx options specify how a container handles duplicates. Other than the AO2_CONTAINER_ALLOC_OPT_DUPS_ALLOW option, the container must be sorted. ao2_unlink() only unlinks the object once if it is in the container. If it is in the container several times, you must ao2_unlink it again.
An object can have the same container listed in its list of weak containers. Once for every time it is in that container. I'm only talking about a simple list just like you are doing with the container nodes. I'm not talking about an ao2 container to hold them as that would be a lot of overhead and overkill.
Object is a member of these weak reference containers:
channels
stasis-controlled
channels
The above object is in the channels container twice and once in the stasis-controlled container. When the object is destroyed it unlinks itself from all containers listed.</pre>
<br />
<p>- rmudgett</p>
<br />
<p>On July 7th, 2014, 11:43 p.m. CDT, 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, Corey Farrell and rmudgett.</div>
<div>By George Joseph.</div>
<p style="color: grey;"><i>Updated July 7, 2014, 11:43 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;">Weak Reference Containers are hash containers that don't maintain references to the objects they contain.
Basically, ao2_link() doesn't increase the ref count and ao2_unlink() doesn't decrease it.
Sounds simple but because the container doesn't have a reference to the object, it's entirely possible that the objects could be destroyed while still in the container. Therefore much of the work in this patch is making sure that if the object is destroyed, it's properly cleaned out of any weak reference containers that it may have been in.
This patch is almost 6000 lines but half of it is units tests...functional, performance, and stress.
Richard: I apologize in advance...I undid some of the refactor undos you had me do earlier. :)</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;">A whole slew of unit tests were added and the existing astobj2 tests were modified to also test weak containers. All passed.
A torture test was added that tests multiple threads accessing multiple containers for weak, hash and rbtree containers. All passed.
The full Testsuite was run with the same number of tests passing that passed before the patch.
</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/utils/Makefile <span style="color: grey">(418136)</span></li>
<li>branches/12/tests/test_astobj2_weak.c <span style="color: grey">(PRE-CREATION)</span></li>
<li>branches/12/tests/test_astobj2_torture.c <span style="color: grey">(PRE-CREATION)</span></li>
<li>branches/12/tests/test_astobj2_thrash.c <span style="color: grey">(418136)</span></li>
<li>branches/12/tests/test_astobj2.c <span style="color: grey">(418136)</span></li>
<li>branches/12/main/astobj2_weak.c <span style="color: grey">(PRE-CREATION)</span></li>
<li>branches/12/main/astobj2_rbtree.c <span style="color: grey">(418136)</span></li>
<li>branches/12/main/astobj2_private.h <span style="color: grey">(418136)</span></li>
<li>branches/12/main/astobj2_hash_private.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>branches/12/main/astobj2_hash.c <span style="color: grey">(418136)</span></li>
<li>branches/12/main/astobj2_container_private.h <span style="color: grey">(418136)</span></li>
<li>branches/12/main/astobj2_container.c <span style="color: grey">(418136)</span></li>
<li>branches/12/main/astobj2.c <span style="color: grey">(418136)</span></li>
<li>branches/12/include/asterisk/astobj2.h <span style="color: grey">(418136)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/3716/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>