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

[Corey Farrell]

> On Dec. 9, 2014, 7:36 p.m., rmudgett wrote:
> > 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.

What about exposing a field 'void *data;' to the weak objects?  This would allow containers of weak proxy objects to make a copy of the data needed for some or all callbacks.  Object types could sacrifice some memory in exchange for speed of container operations.  Honestly I'm not sure if this would be a good thing or not.


> On Dec. 9, 2014, 7:36 p.m., rmudgett wrote:
> > /trunk/main/astobj2.c, lines 798-804
> > <https://reviewboard.asterisk.org/r/4108/diff/3/?file=68553#file68553line798>
> >
> >     Use of ao2_bump here is unnecessary since you already checked if the pointer was NULL.

Thanks for pointing out.  I tend to forget the overhead of the NULL check in ao2_bump and use it for the return value.  Fixed.


> On Dec. 9, 2014, 7:36 p.m., rmudgett wrote:
> > /trunk/tests/test_astobj2_weaken.c, line 118
> > <https://reviewboard.asterisk.org/r/4108/diff/3/?file=68554#file68554line118>
> >
> >     This should be fail_cleanup1 so obj gets cleaned up.

This is a paranoia check - the test only passes if all conditions of the if statement are false.  If obj were somehow freed early all conditions would be true.  In that case obj points to an object that was already destroyed.  I feel the best thing is for this test to risk leaking in this situation, since the other option would be to risk a crash.


> On Dec. 9, 2014, 7:36 p.m., rmudgett wrote:
> > /trunk/tests/test_astobj2_weaken.c, lines 91-96
> > <https://reviewboard.asterisk.org/r/4108/diff/3/?file=68554#file68554line91>
> >
> >     Use of weakref1 after ref released.

This is intentional.  I'm checking the pointer without dereferencing it, so this is ok.


> On Dec. 9, 2014, 7:36 p.m., rmudgett wrote:
> > /trunk/main/astobj2.c, line 432
> > <https://reviewboard.asterisk.org/r/4108/diff/3/?file=68553#file68553line432>
> >
> >     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.

I've transfered the actual creation of weak proxy objects from ao2_weaken to ao2_alloc (with an option).


- Corey


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


On Oct. 26, 2014, 7: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, 7: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/20150205/eb5ddffb/attachment.html>