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