[asterisk-dev] [Code Review] 3716: Weak Reference Containers

[George Joseph]

> On July 15, 2014, 2 p.m., Corey Farrell wrote:
> > The ability to add an object to multiple weak reference containers is concerning to me.  I think this adds unnecessary complexity/risk to astobj2.  If you could tell me a use case where this amount of freedom is required I might be convinced otherwise.  Otherwise I'd feel much better with a more limited ability to use weak reference containers:
> > 
> > 1. Only allow the caller of ao2_alloc to add the object to a weak container, and only one container.
> > 2. Insertion to the weak container should only happen before the object is reachable outside the local scope.
> > 3. The weak container would be a private detail to the unit that owns it.  All other code would use a method to retrieve references from that container.
> > 
> > These two rules would greatly reduce the possibility of races, which reduces the amount of work needed to avoid races.  In particular rule #2 would allow ao2_ref to only perform locking during ao2_ref(obj, -1).  I had started work on this in r3463 but discarded it when you started splitting astobj2.c.

We're going to disgree. :)  I think this is a pretty severe restriction and it would be more complex to enforce the restriction than to safely allow it.  How about this...  If you can break it and I can't reasonably fix it, I'll back off.   I know you're busy so just write a pseudo code test case, or just give me an idea for one and I'll create a unit test for it.


- George


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3716/#review12583
-----------------------------------------------------------


On July 7, 2014, 10: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, 10: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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140715/f87a91c6/attachment.html>