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