[asterisk-dev] [Code Review] 4108: Weak Proxy Objects

[Corey Farrell]

> On March 31, 2015, 5:52 p.m., rmudgett wrote:
> > /trunk/main/astobj2.c, line 798
> > <https://reviewboard.asterisk.org/r/4108/diff/5/?file=71780#file71780line798>
> >
> >     I think you need to have a weakproxy destructor callback that performs a sanity check on the proxy object to ensure that it is not still linked with a real object.  
> >     
> >     Or even better, make the ao2_ref code do this check when it is about to destroy a weakproxy object.  This check can also destroy anything in the notify list for the case if the real object was not set but subscribers were.

Once linked to a real object the weakproxy cannot be destroyed until after no references to the real object exist.  Remember that until the real object is destroyed it holds a reference to the weakproxy.

As for subscriptions I will have to look into this more, but my intention for subscriptions is a way to request notification as soon as the weakproxy points to NULL.  So in the case of adding a subscription when no real object is linked, I think we should run the callback immediately and not add it to the subscription list.


> On March 31, 2015, 5:52 p.m., rmudgett wrote:
> > /trunk/main/astobj2.c, lines 450-451
> > <https://reviewboard.asterisk.org/r/4108/diff/5/?file=71780#file71780line450>
> >
> >     Isn't the shell game you are playing with the real object's ref count going to mess up the DEBUG_REF output?
> >     
> >     How about this code:
> >     Code assumes that weakproxy ref wasn't bumped when set above.
> >     if weakproxy
> >        if current_value == 1
> >           unlink real and weak
> >        unlock weakproxy
> >        if current_value == 1
> >           run weakproxy callbacks (locking between each callback or locking to run all)
> >           unref weakproxy
> >           unref real

Lets use realobj to represent the variable in the callers scope (non-ao2 code), user_data is the variable in internal_ao2_ref.  This shell game results in REF_DEBUG logs showing the 2nd to last reference being released by ao2_ref(user_data, -1) a couple lines below your finding, just prior to the user releasing the last reference.  REF_DEBUG is produced after internal_ao2_ref completes, so it's impossible for the unref's in this block to be logged after ao2_ref(realobj, -1).  So without this REF_DEBUG would say that the object was destroyed due to ao2_ref(user_data, -1) before the 2nd to last reference was released.

This REF_DEBUG output was verified using the provided test.


- Corey


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


On March 4, 2015, 4:43 p.m., Corey Farrell wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4108/
> -----------------------------------------------------------
> 
> (Updated March 4, 2015, 4:43 p.m.)
> 
> 
> Review request for Asterisk Developers, George Joseph and rmudgett.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This implements "weak" references.  The weakproxy object is a real ao2 with normal reference counting of its own.  When a weakproxy is pointed to a normal object they hold references to each other.  The normal object is automatically freed when a single reference remains (the weakproxy).  The weakproxy also supports subscriptions that will notify callbacks when the normal object is about to be destroyed.
> 
> 
> Diffs
> -----
> 
>   /trunk/tests/test_astobj2_weaken.c PRE-CREATION 
>   /trunk/main/astobj2.c 432445 
>   /trunk/include/asterisk/astobj2.h 432445 
> 
> Diff: https://reviewboard.asterisk.org/r/4108/diff/
> 
> 
> Testing
> -------
> 
> Ran the included test with REF_DEBUG enabled under valgrind.  No reference leaks or improper memory access.  Though this does not test for races, I don't know of an automated way to do that.
> 
> 
> Thanks,
> 
> Corey Farrell
> 
>

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