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

[rmudgett]

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


I'm seeing several race conditions dealing with when the object is destroyed in relation to container traversal operations.  I'm not seeing anything to prevent an object from being destroyed after a traversal has selected an object but before it has bumped the ref or decided what it's going to do with it.

Container destruction race and object destruction race.
Container unlink race and object destruction race.
Container search race and object destruction race.


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

    Move this to after getting the spin lock since old_value is effectively a cached value that could change outside of the spin lock protection.



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

    Revert this because setting old_value should be moved to after spin lock protection.  Without any protection, the value could potentially be invalid unless the architecture does an atomic read.



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

    You cannot unref a container's node without the container being write locked.  The node's destructor unlinks it from the container's structure.
    
    The weak ref list must be cleaned up before calling the object's destructor because the container can use the hash_fn, sort_fn, and cmp_fn callbacks on destroying objects.
    
    I think this issue may be difficult to fix.



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

    This swap cannot work.  The nodes can be in only one list at a time because there is only one list next pointer.  You have to have the two spinlocks at the same time to be safe.  In addition the existing object could be in the process of being destroyed so the optimization of swaping objects in the node is not safe.  You would need to unlink the old node and link the new node in its place.
    
    You've done this swap in several places.


- rmudgett


On April 30, 2014, 5:13 p.m., George Joseph wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3513/
> -----------------------------------------------------------
> 
> (Updated April 30, 2014, 5:13 p.m.)
> 
> 
> Review request for Asterisk Developers, Joshua Colp and rmudgett.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Add weak reference capability to hash and list astobj2 containers.
> 
> Current (Strong-reference) behavior:  A container (list, hash or rbtree) increments an object's ref count when linked and decrements an object's ref count when unlinked.  Therefore the object can never be destroyed while it is an entry in a container unless someone accidentally decrements the object's ref count too many times.  In this case, the object is invalid but the container doesn't know it.  
> 
> Weak-reference behavior:  A container (list or hash) allocated with the option AO2_CONTAINER_ALLOC_OPT_WEAK_REF neither increments an object's ref count when linked nor decrements an object's ref count when unlinked.  The object's ref count can therefore validly reach 0 while still in a weak-ref container in which case the object is automatically and cleanly removed from any weak-ref container it may be in.  Because of the complexity of the red-black tree implementation, it isn't eligible for weak-reference behavior.
> 
> Use case:  The possible need for weak-ref containers came up during the development of the Sorcery registry.  The first call to sorcery_open from a module should create a new sorcery instance and subsequent calls from that same module should use that instance.  The last call to close should free that instance.  With a strong-ref container however, the container itself always has a reference to the instance so it doesn't get destroyed without special code to check the ref count on each call to close.  The same thing happens with the pjsip cli command registry.
> 
> Implementation:  astobj2.__priv_data now has a linked list that contains the weak-ref container nodes that reference the object.  When an object is added to a weak-ref container, the container node is added to the list instead of the node incrementing the object's ref count.  Similarly, when an object is removed from a weak-ref container the node is removed from the linked list instead of the object's ref count being decremented.  If an object's ref count reaches 0 the object's linked list is traversed and the corresponding nodes are cleared effectively removing the object from the container.  NOTE:  An object's ref count is still incremented as the result of a retrieval (find, iterate, etc) from a weak-ref container.
> 
> Backwards compatibility:  Unless the AO2_CONTAINER_ALLOC_OPT_WEAK_REF flag was set on container allocation, all container operations remain exactly as they are today and nothing prevents an object from being a member of both strong and weak ref containers at the same time.
> 
> Performance implications:  Due to code reorganization, the performance of strong-ref containers is actually microscopically BETTER than the unmodified code and the performance of weak-ref containers is even better than that.  In other words, the performance of the default behavior was not penalized by the addition of the new feature.  The test framework now has a /main/astobj2/perf/ category to show relative performance.  NOTE:  Previously, when the test framework was enabled AO2_DEBUG was also enabled but this was skewing the results so I removed that auto-enablement of AO2_DEBUG.  The test framework never needed it.
> 
> The original RFC review request was here... https://reviewboard.asterisk.org/r/3382/
> The major changes from the RFC are that a spinlock was added around the object's ref-counter operations and the rbtree's are now excluded from being weak-reference containers.
> 
> 
> Diffs
> -----
> 
>   branches/12/tests/test_astobj2_thrash.c 413157 
>   branches/12/tests/test_astobj2.c 413157 
>   branches/12/main/astobj2.c 413157 
>   branches/12/include/asterisk/astobj2.h 413157 
> 
> Diff: https://reviewboard.asterisk.org/r/3513/diff/
> 
> 
> Testing
> -------
> 
> 7 new tests were added to the existing astobj2 test framework.  All tests pass.
> 
> The testsuite is a little more challenging but I at least made sure that all the test that passed before the change still passed after the change.
> 
> 
> Thanks,
> 
> George Joseph
> 
>

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