> 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.
>
> rmudgett wrote:
> 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.
>
> George Joseph wrote:
> I understand how the containers work, it's linked lists that are the
> issue. The container needs a list entry structure to participate in a linked
> list, no? That means that that container can only be in 1 linked list that
> uses that list entry at a time. Hence it can't be in more than 1 object's
> list at a time.
>
>
> George Joseph wrote:
> Let me rephrase... the container can't be in multiple object's lists.
>
>
> rmudgett wrote:
> A list entry node needs to be allocated for every time it is put on an
> objects weak container membership list.
>
> Allocate a struct like this for each entry added to the objects
> membership list when it is added.
>
> struct weak_membership_node {
> AST_LIST_ENTRY(weak_membership_node) next;
> /*! Holds a reference to the weak container the object is in. */
> struct ao2_container *member_of;
> }
>
>
>
> George Joseph wrote:
> How is this any better than just using the node? Concurrency issues
> aside, it's already allocated and it saves having to do a traverse on the
> container just to find it and unlink it.
It does not increase module coupling/complexity by entangling itself with the
lifetime of the container node object.
- 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
