[asterisk-dev] [Code Review] 3463: RFC: astobj2 cached objects (alternative to weak containers)

[rmudgett]

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


{quote}
One question of safety I have, do I need to use atomic operations to get/set astobj2->priv_data.cached_in?
{quote}

I think you do.  This problem has a mutual reference condition that could be ended by either party.  The big problem is when both sides decide to end the condition at the same time.


/trunk/main/astobj2.c
<https://reviewboard.asterisk.org/r/3463/#comment21468>

    This will cause a recursive lock of the container when the object is unlinked.  You should clear the priv_data.cached_in pointer before unlinking.
    
    Nevermind.  I now see the pointer clearing in the traversal.



/trunk/main/astobj2.c
<https://reviewboard.asterisk.org/r/3463/#comment21469>

    Setting cached_in to NULL is not needed.



/trunk/main/astobj2.c
<https://reviewboard.asterisk.org/r/3463/#comment21472>

    This could be made a property of the cache container rather than a separate call.  Then you can just link an object into the container to cache it.
    
    Cache containers must be declared with a mutex/rwlock.
    
    Cache containers cannot link objects that are already in a cache container.



/trunk/main/astobj2.c
<https://reviewboard.asterisk.org/r/3463/#comment21471>

    I think you have a reference problem with the priv_data.cached_in container.
    
    Unreffing the object until it drops to one ref while it is cached is ok.
    
    Manually unlinking the cached object from the cache container has an unbalanced unref to the container.


- rmudgett


On April 17, 2014, 6:09 p.m., Corey Farrell wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3463/
> -----------------------------------------------------------
> 
> (Updated April 17, 2014, 6:09 p.m.)
> 
> 
> Review request for Asterisk Developers, George Joseph, Joshua Colp, and rmudgett.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This patch is not ready to be committed, I'm posting it now to determine if this feature is wanted.  I realize that I have ignored REF_DEBUG tags in many places, that will be taken care of if it is decided that we should proceed with this feature (same with documentation).
> 
> This introduces possible locking from inside ao2_ref, but only when unreferencing an object that is cached.  Safe use of container cache's require that they be locked when retrieving or removing objects.  Since cached objects hold a reference to the cache, this is needed for safety.  This means that a container can't be freed until it is cleared of cached objects.
> 
> ao2_cache_enable is meant to run immediately after ao2_alloc to add a new object to it's cache.  I don't care much for the procedure name, I welcome suggestions.
> 
> I realize this is more limited than George's original proposal of being able to add objects to any number of weak containers.  Supporting multiple containers makes it impossible for astobj2.c to safely lock all weak reference owners at the same time, and without that lock it's possible for another thread to retrieve a reference to the object you thought you were destroying.  Given the additional complexity and risk of supporting multiple weak containers, I have to ask why would we need that?
> 
> One question of safety I have, do I need to use atomic operations to get/set astobj2->priv_data.cached_in?
> 
> 
> Diffs
> -----
> 
>   /trunk/main/astobj2.c 412466 
>   /trunk/include/asterisk/astobj2.h 412466 
> 
> Diff: https://reviewboard.asterisk.org/r/3463/diff/
> 
> 
> Testing
> -------
> 
> Only ran tests from tests_astobj2.so, no new tests written for cached objects.  I haven't done any real tests of caching an object in a container.
> 
> 
> Thanks,
> 
> Corey Farrell
> 
>

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