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

[Corey Farrell]

> On March 31, 2015, 5:52 p.m., rmudgett wrote:
> > This approach is much simpler and easier to understand than previous attempts.  Since it is more general it could be used to avoid mutual ref issues such as between the stasis_topic and stasis_subscription as described in main/stasis.c.
> > 
> > All of the issues I've pointed out are minor.

With that said I've updated the API to include REF_DEBUG parameters.

One thing with REF_DEBUG, I don't think there is a reasonable way to provide procedures that don't accept tag/file/line/func parameters.  The real overhead of REF_DEBUG comes from fprintf / fflush, so I don't think this matters.  Especially given that internal_ao2_ref already takes 3 of the 4 parameters.

I think it's worth looking at getting rid of the dual ABI of ao2 (will need to benchmark).  It would be useful for example to have REF_DEBUG information known by INTERNAL_OBJ so we get more useful error messages.  It would also simplify code by removing a lot of the #if/#else blocks throughout Asterisk.


> 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.
> 
> Corey Farrell wrote:
>     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.

I've moved forward with my plan to make callbacks run immediately when weakproxy points to NULL.  One use I envision for subscriptions is for ao2_containers.  The weakproxy will be linked to a container, then a subscription will be created to unlink weakproxy when it no longer points to a real object.  If it already doesn't point to a real object then it should be immediately removed.


> 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
> 
> Corey Farrell wrote:
>     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.

REF_DEBUG output for all objects in test_astobj2_weaken.c is posted to JIRA ticket.  I believe this is the desired outcome.


> On March 31, 2015, 5:52 p.m., rmudgett wrote:
> > /trunk/include/asterisk/astobj2.h, line 692
> > <https://reviewboard.asterisk.org/r/4108/diff/5/?file=71779#file71779line692>
> >
> >     Why not use the OBJ_NOLOCK flag?

I've also added support for OBJ_MULTIPLE in the flag to remove all matching subscriptions.


- 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/20150403/b509a6a5/attachment-0001.html>