[asterisk-dev] [Code Review] 4108: Weak References

[rmudgett]

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


Summary of this weak ref implementation:
weak_proxy_obj <-> obj

* The weak_proxy_obj and the obj get refs to each other on the initial call to ao2_weaken().
* The weak_proxy_obj will then stick around for as long as obj lives so it can be returned by subsequent ao2_weaken() calls.
* Once obj dies because its last external ref is released, the weak_proxy_obj will die when the last external ref to it is released.

This weak object strategy is workable for random objects.  It seems rather expensive for use by keyed containers because the real obj needs to be referenced by container operations through the sort_fn, hash_fn, and cmp_fn callbacks.


/trunk/main/astobj2.c
<https://reviewboard.asterisk.org/r/4108/#comment24459>

    It is better to use a mask on the magic number struct member to ignore the stolen bit rather than to compare two different magic numbers.
    
    if (AO2_MAGIC != (obj->priv_data.magic & AO2_MAGIC_MASK)) { Not a valid object }
    
    if (obj->priv_data.magic & AO2_WEAK_OBJ) { is weak object code }



/trunk/main/astobj2.c
<https://reviewboard.asterisk.org/r/4108/#comment24458>

    Use of obj->priv_data.weakptr is not protected from other threads creating a weak object from the object.  This wouldn't be a problem if the object were required to be immediately weakened after creation.



/trunk/main/astobj2.c
<https://reviewboard.asterisk.org/r/4108/#comment24460>

    The safe traversal macro is a relatively expensive way of doing this:
    
    while ((destroyed_cb = AST_LIST_REMOVE_HEAD(&weak->destroyed_cb)) {
      destroyed_cb->cb()
      ast_free(destroyed_cb)
    }
    



/trunk/main/astobj2.c
<https://reviewboard.asterisk.org/r/4108/#comment24461>

    Use of ao2_bump here is unnecessary since you already checked if the pointer was NULL.



/trunk/main/astobj2.c
<https://reviewboard.asterisk.org/r/4108/#comment24463>

    Same for ao2_bump use here.



/trunk/main/astobj2.c
<https://reviewboard.asterisk.org/r/4108/#comment24465>

    ao2_bump is not necessary since obj is never NULL.



/trunk/tests/test_astobj2_weaken.c
<https://reviewboard.asterisk.org/r/4108/#comment24456>

    Use of weakref1 after ref released.



/trunk/tests/test_astobj2_weaken.c
<https://reviewboard.asterisk.org/r/4108/#comment24457>

    This should be fail_cleanup1 so obj gets cleaned up.


- rmudgett


On Oct. 26, 2014, 6:10 a.m., Corey Farrell wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4108/
> -----------------------------------------------------------
> 
> (Updated Oct. 26, 2014, 6:10 a.m.)
> 
> 
> Review request for Asterisk Developers, George Joseph and rmudgett.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This implements weak references.  The weak object is a real ao2 with normal reference counting of its own.  The original object is destroyed when the only reference remaining is from the weak object.
> 
> 
> Diffs
> -----
> 
>   /trunk/tests/test_astobj2_weaken.c PRE-CREATION 
>   /trunk/main/astobj2.c 426139 
>   /trunk/include/asterisk/astobj2.h 426139 
> 
> Diff: https://reviewboard.asterisk.org/r/4108/diff/
> 
> 
> Testing
> -------
> 
> Very little, I'm unsure how to actually test that this cannot race, since any potential for a race would be due to very exact timing.
> 
> 
> Thanks,
> 
> Corey Farrell
> 
>

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