> On July 24, 2014, 4:01 p.m., rmudgett wrote: > > 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. > > George Joseph wrote: > 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.
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. - rmudgett ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3716/#review12860 ----------------------------------------------------------- On July 7, 2014, 11:43 p.m., George Joseph wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/3716/ > ----------------------------------------------------------- > > (Updated July 7, 2014, 11:43 p.m.) > > > Review request for Asterisk Developers, Corey Farrell and rmudgett. > > > Repository: Asterisk > > > Description > ------- > > 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. :) > > > Diffs > ----- > > branches/12/utils/Makefile 418136 > branches/12/tests/test_astobj2_weak.c PRE-CREATION > branches/12/tests/test_astobj2_torture.c PRE-CREATION > branches/12/tests/test_astobj2_thrash.c 418136 > branches/12/tests/test_astobj2.c 418136 > branches/12/main/astobj2_weak.c PRE-CREATION > branches/12/main/astobj2_rbtree.c 418136 > branches/12/main/astobj2_private.h 418136 > branches/12/main/astobj2_hash_private.h PRE-CREATION > branches/12/main/astobj2_hash.c 418136 > branches/12/main/astobj2_container_private.h 418136 > branches/12/main/astobj2_container.c 418136 > branches/12/main/astobj2.c 418136 > branches/12/include/asterisk/astobj2.h 418136 > > Diff: https://reviewboard.asterisk.org/r/3716/diff/ > > > Testing > ------- > > 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. > > > Thanks, > > George Joseph > >
-- _____________________________________________________________________ -- 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
