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

rmudgett reviewboard at asterisk.org
Thu Jul 24 16:01:01 CDT 2014 

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


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.


branches/12/main/astobj2.c
<https://reviewboard.asterisk.org/r/3716/#comment23222>

    You need to separate the magic number bits from the state bits.  The magic number bits should never change.
    
    As it is you could have a thread check an object for validation and not see a valid value because it was in the process of being updated.
    



branches/12/main/astobj2.c
<https://reviewboard.asterisk.org/r/3716/#comment23223>

    Keeping the magic number at the end of the struct provided some protection from underwrites performed on the user data.



branches/12/main/astobj2.c
<https://reviewboard.asterisk.org/r/3716/#comment23230>

    This function's purpose is a falacy.  It's result is valid only for the instant before it returns.  Once it releases the spin lock the object could be destroyed by another thread.  Any decisions made by code after it returns are suspect if the object is returned as "valid".



branches/12/main/astobj2.c
<https://reviewboard.asterisk.org/r/3716/#comment23231>

    Destroy the object's user data AFTER it has been removed from all weak containers.  Weak containers can still access the user data with the container callbacks during traversals.  __is_ao2_object_healthy() does not prevent the access.



branches/12/main/astobj2.c
<https://reviewboard.asterisk.org/r/3716/#comment23227>

    Use !AST_LIST_EMPTY()



branches/12/main/astobj2.c
<https://reviewboard.asterisk.org/r/3716/#comment23229>

    AST_LIST_TRAVERSE_SAFE_BEGIN does not provide safety from other threads.  It provides safety from actions within the loop affecting the list's current node only.
    
    Also using the node->my_container pointer here is unsafe.  That pointer does not poses a reference to the container and the container node can out live the container at this point.
    



branches/12/main/astobj2.c
<https://reviewboard.asterisk.org/r/3716/#comment23225>

    This comment belongs a couple lines up where the state is assigned AO2_DEAD.



branches/12/main/astobj2_weak.c
<https://reviewboard.asterisk.org/r/3716/#comment23228>

    The weak container needs its own destructor function to mitigate the race if the container destructor happens at the same time as an object in the container is being destroyed.
    
    Either it needs to assert that the container is empty.
    or
    1) It needs to unlink all objects in the container as normal.
    2) It needs to wait for any pending object destructor collision to complete.
    3) It needs to check that there are no more nodes remaining for the normal reference leak check.  This can probably be done at the same time as waiting for the pending destructor collision nodes to go away.
    


- rmudgett


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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140724/953d47e6/attachment-0001.html>

More information about the asterisk-dev mailing list