----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3716/#review12583 -----------------------------------------------------------
The ability to add an object to multiple weak reference containers is concerning to me. I think this adds unnecessary complexity/risk to astobj2. If you could tell me a use case where this amount of freedom is required I might be convinced otherwise. Otherwise I'd feel much better with a more limited ability to use weak reference containers: 1. Only allow the caller of ao2_alloc to add the object to a weak container, and only one container. 2. Insertion to the weak container should only happen before the object is reachable outside the local scope. 3. The weak container would be a private detail to the unit that owns it. All other code would use a method to retrieve references from that container. These two rules would greatly reduce the possibility of races, which reduces the amount of work needed to avoid races. In particular rule #2 would allow ao2_ref to only perform locking during ao2_ref(obj, -1). I had started work on this in r3463 but discarded it when you started splitting astobj2.c. - Corey Farrell On July 8, 2014, 12:43 a.m., George Joseph wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/3716/ > ----------------------------------------------------------- > > (Updated July 8, 2014, 12:43 a.m.) > > > Review request for Asterisk Developers, Corey Farrell and rmudgett. > > > Repository: Asterisk > > > Description > ------- > > Weak Reference Containers are hash containers that don't maintain references > to the objects they contain. > Basically, ao2_link() doesn't increase the ref count and ao2_unlink() doesn't > decrease it. > > Sounds simple but because the container doesn't have a reference to the > object, it's entirely possible that the objects could be destroyed while > still in the container. Therefore much of the work in this patch is making > sure that if the object is destroyed, it's properly cleaned out of any weak > reference containers that it may have been in. > > This patch is almost 6000 lines but half of it is units tests...functional, > performance, and stress. > > Richard: I apologize in advance...I undid some of the refactor undos you had > me do earlier. :) > > > Diffs > ----- > > branches/12/utils/Makefile 418136 > branches/12/tests/test_astobj2_weak.c PRE-CREATION > branches/12/tests/test_astobj2_torture.c PRE-CREATION > branches/12/tests/test_astobj2_thrash.c 418136 > branches/12/tests/test_astobj2.c 418136 > branches/12/main/astobj2_weak.c PRE-CREATION > branches/12/main/astobj2_rbtree.c 418136 > branches/12/main/astobj2_private.h 418136 > branches/12/main/astobj2_hash_private.h PRE-CREATION > branches/12/main/astobj2_hash.c 418136 > branches/12/main/astobj2_container_private.h 418136 > branches/12/main/astobj2_container.c 418136 > branches/12/main/astobj2.c 418136 > branches/12/include/asterisk/astobj2.h 418136 > > Diff: https://reviewboard.asterisk.org/r/3716/diff/ > > > Testing > ------- > > A whole slew of unit tests were added and the existing astobj2 tests were > modified to also test weak containers. All passed. > A torture test was added that tests multiple threads accessing multiple > containers for weak, hash and rbtree containers. All passed. > The full Testsuite was run with the same number of tests passing that passed > before the patch. > > > Thanks, > > George Joseph > >
-- _____________________________________________________________________ -- 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
