[Asterisk-code-review] ao2 container: Add support for transparent storage of weakpr... (asterisk[master])

[Richard Mudgett]

Richard Mudgett has posted comments on this change.

Change subject: ao2_container: Add support for transparent storage of weakproxy objects.
......................................................................


Patch Set 2: Code-Review-1

> Richard,
 > 
 > Would love you're opinion on this patch.  I'm not really happy with
 > the API but I'm not sure how to fix.
 > 
 > Goals:
 > 1) Simplify callers of ao2_find / unify code for retrieving from
 > weakproxy in container.
 > 2) Automate weakproxy subscription for ao2_link.
 > 3) Avoid taking the reference bump/release of node->obj, directly
 > return a reference to ao2_weakproxy_get_object(node->obj).
 > 
 > 
 > Questions/Concerns:
 > 1) The need to check ao2_weakproxy_ref_object() in the
 > named_locks_cmp is ugly.  I'd love if this could just be handled by
 > internal_ao2_traverse, but I'm not sure a good way to abort
 > CMP_MATCH in the case where weakproxy has no object.

The callback should not do the check because you are lying to the container
and the check code is generic anyway.  To ignore the CMP_MATCH in the
container traversal code you just need to continue to the next iteration
after finding out that the object is empty.  The traversal code would need
to determine if this is intended to unlink the specific weak proxy object
though.  i.e., We meant to find the weak proxy object to remove it from
the container because the real object got destroyed.

An alternate view is that if you found an empty proxy object then there
isn't a real object to be found even if you ignored the empty proxy.

 > 2) Do we want this to be an alloc option, or flags given to
 > ao2_link, ao2_find and ao2_callback?

I think this should be a container option flag at container allocation
time as you have already started.  The container callback functions would
deal with the proxy object.  The container will be given normal ao2
objects and return normal ao2 objects just like a normal container.  The
difference being that the container would not hold a reference to the ao2
object.  The only caveat would be that the stored ao2 objects must already
have an associated proxy object.  The container would handle the weak
proxy subscribe/unsubscribe and the subscribe notification callback would
need to be an internal unlink function.  The container code would need to
check the option flag when it needs to do weak proxy actions.

Some thought is needed to determine if weak containers can support
AO2_ALLOC_OPT_LOCK_RWLOCK.  There might be a reentrancy locking problem
with AO2_ALLOC_OPT_LOCK_RWLOCK within the container traversal code.

AO2_ALLOC_OPT_LOCK_NOLOCK weak containers are possible but the real
objects must also be controlled by the owning thread.  Weak proxy
notification callbacks could happen at any time if other threads have
references to the real object.  The added restrictions for
AO2_ALLOC_OPT_LOCK_NOLOCK weak containers really negate the benefits of
reference counting altogether.

A weakness of the proxy object is that any container keys in the proxy
must be immutable.  The code should not need to know which containers the
proxy is a member.  As an extension, the keys in the real ao2 object must
be immutable (if the real object even has a copy of the keys in the proxy
object).

 > 3) How to deal with manual ao2_unlink?

I don't see a problem.  Part of any weak proxy unlinking from the
container is to unsubscribe from the weak proxy notifications.

However, the automatic unlink is a problem if the proxy object is in the
container multiple times.  The weakproxy_run_callbacks() would need to run
through the list calling the callbacks and then delete the list as two
separate loops.  It needs to be two loops because the callback for the
weak proxy container would try to unsubscribe and if it were already
removed then the second subscription would be removed and orphan the
second weak proxy object in the container.

 > 4) How to deal with shutdown (cleanup of non-empty container)?

I don't see a problem.  The container would just unlink any objects it
contains just like it currently does.  Part of any weak proxy unlinking
from the container is to unsubscribe from the weak proxy notifications.

 > 5) Do we need to deal with the possibility that a weakproxy can be
 > associated with a new object before node_obj_proxy_cb() is called? 
 > Current patch does not support reuse of weakproxy objects added to
 > container due to this.

Weak proxy objects are only associated with one object at a time.  They
can only get unassociated when the associated object gets destroyed and
the notification subscription callbacks get called.  There isn't a weak
proxy unset function to disassociate an object from the weak proxy since
it couldn't be thread safe when altering the real object's proxy pointer.

-- 
To view, visit https://gerrit.asterisk.org/3749
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I48f2dfb85e97fd0a382a869cd084afda129c95d9
Gerrit-PatchSet: 2
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: No