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

[Corey Farrell]

Corey Farrell has posted comments on this change.

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


Patch Set 2:

Thank you for your advice Richard, hopefully I'll be able to address most of this in the next week.

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

Understood.

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

Locking order can cause the old object to still exist after a new one is linked.  Flow:
Thread 1: acquire container lock in ast_named_lock_get
Thread 2: blocked for container lock in ast_named_lock_get
Thread 3: last reference to named lock "aor/alice" released, node_obj_proxy_cb blocks at ao2_unlink.
Thread 1: ao2_find returns NULL, create and link new named_lock "aor/alice".  release container lock.
Thread 2: container lock acquired, ao2_find traversal order finds old "aor/alice" first but must skip.

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

So ao2_link/ao2_unlink would take the real object for it's parameter.   I feel like the weakproxy object (or at least the struct type) should normally be used as 'arg' for ao2_find/ao2_callback when OBJ_POINTER / OBJ_SEARCH_OBJECT are used?

It was already intentional that weakproxy containers would not (can not) accept unassociated objects.  In the reverse direction if we were giving the weakproxy to ao2_link this would cause ao2_weakproxy_subscribe to run the callback immediately.

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

I'm not sure it's worth the risk to allow/support this?  I think the normal usage pattern for weakproxy containers would be: lock the container, do a find, if find returns NULL allocate a new object and insert it, then unlock the container.  So for the ao2_find you would still need a write lock since a new object will be inserted if non was available.  Question is do we document a big warning about this or block it in container allocation (error out)?

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

NOLOCK would not be useful or safe and most likely should be blocked.  Long term maybe someone will have a use for giving the container a lockobj, but that's out of scope for this commit.

 > A weakness of the proxy object is that any container keys in the
 > proxy must be immutable.

I'm not sure how this is different from standard containers?  I thought any key that can be used by container callbacks must be immutable as long as the object is in any container.

 > The code should not need to know which containers the
 > proxy is a member.

I don't understand this comment.  Are you talking about not giving the container reference to ao2_weakproxy_subscribe?

 > 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).

Actually ao2 weakproxy design had this in mind.  The correct pattern for keys on weak and real objects is:
struct myobj_proxy {
  AO2_WEAKPROXY();
  const char key[0];
};

struct myobj {
  const char *key;
};

myobj->key would be a pointer to myobj_proxy->key.  This is safe once the ao2 weakproxy association is made, it is impossible for myobj_proxy to be destroyed before myobj, so immutable strings on the weakproxy should never be duplicated to the real 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.

Maybe ao2_weakproxy_subscribe should be passed a reference to 'node' instead of 'self'? This way each ao2_link would have a unique subscription.

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

This actually is currently a problem, but I think my idea for #3 will solve it.  Currently ao2_weakproxy_subscribe gets a bumped reference to the container, so it is a circular reference.

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

I actually misstated my concern.  By the time node_obj_proxy_cb is called the weakproxy is already unassociated.  ao2_unlink will block if another thread has the container locked, that other thread could then associate a new real object to the weakproxy.  This is a very unlikely execution flow, maybe not even worth worrying about.  named_locks.c cannot have this problem since it makes no attempt to reuse the weakproxy objects.

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