[Asterisk-code-review] astobj2: Run weakproxy callbacks outside of lock. (asterisk[master])

[Jenkins2]

Jenkins2 has submitted this change and it was merged. ( https://gerrit.asterisk.org/6733 )

Change subject: astobj2: Run weakproxy callbacks outside of lock.
......................................................................

astobj2: Run weakproxy callbacks outside of lock.

Copy the list of weakproxy callbacks to temporary memory so they can be
run without holding the weakproxy lock.

Change-Id: Ib167622a8a0f873fd73938f7611b2a5914308047
---
M include/asterisk/astobj2.h
M main/astobj2.c
2 files changed, 26 insertions(+), 19 deletions(-)

Approvals:
  Richard Mudgett: Looks good to me, but someone else must approve
  Joshua Colp: Looks good to me, but someone else must approve
  George Joseph: Looks good to me, approved
  Jenkins2: Approved for Submit



diff --git a/include/asterisk/astobj2.h b/include/asterisk/astobj2.h
index 484e1e3..9b5ec12 100644
--- a/include/asterisk/astobj2.h
+++ b/include/asterisk/astobj2.h
@@ -671,6 +671,10 @@
  *       of the cb / data pair.  If it was subscribed multiple times it must be
  *       unsubscribed as many times.  The OBJ_MULTIPLE flag can be used to remove
  *       matching subscriptions.
+ *
+ * \note When it's time to run callbacks they are copied to a temporary list so the
+ *       weakproxy can be unlocked before running.  That means it's possible for
+ *       this function to find nothing before the callback is run in another thread.
  */
 int ao2_weakproxy_unsubscribe(void *weakproxy, ao2_weakproxy_notification_cb cb, void *data, int flags);
 
diff --git a/main/astobj2.c b/main/astobj2.c
index d534900..da2d44e 100644
--- a/main/astobj2.c
+++ b/main/astobj2.c
@@ -82,8 +82,6 @@
 	AST_LIST_ENTRY(ao2_weakproxy_notification) list;
 };
 
-static void weakproxy_run_callbacks(struct ao2_weakproxy *weakproxy);
-
 struct ao2_lock_priv {
 	ast_mutex_t lock;
 };
@@ -469,7 +467,7 @@
 	struct astobj2_lockobj *obj_lockobj;
 	int current_value;
 	int ret;
-	void *weakproxy = NULL;
+	struct ao2_weakproxy *weakproxy = NULL;
 
 	if (obj == NULL) {
 		if (ref_log && user_data) {
@@ -498,6 +496,8 @@
 #endif
 
 	if (weakproxy) {
+		struct ao2_weakproxy cbs;
+
 		if (current_value == 1) {
 			/* The only remaining reference is the one owned by the weak object */
 			struct astobj2 *internal_weakproxy;
@@ -508,8 +508,9 @@
 			internal_weakproxy->priv_data.weakptr = NULL;
 			obj->priv_data.weakptr = NULL;
 
-			/* Notify the subscribers that weakproxy now points to NULL. */
-			weakproxy_run_callbacks(weakproxy);
+			/* transfer list to local copy so callbacks are run with weakproxy unlocked. */
+			cbs.destroyed_cb = weakproxy->destroyed_cb;
+			AST_LIST_HEAD_INIT_NOLOCK(&weakproxy->destroyed_cb);
 
 			/* weak is already unlinked from obj so this won't recurse */
 			ao2_ref(user_data, -1);
@@ -518,6 +519,14 @@
 		ao2_unlock(weakproxy);
 
 		if (current_value == 1) {
+			struct ao2_weakproxy_notification *destroyed_cb;
+
+			/* Notify the subscribers that weakproxy now points to NULL. */
+			while ((destroyed_cb = AST_LIST_REMOVE_HEAD(&cbs.destroyed_cb, list))) {
+				destroyed_cb->cb(weakproxy, destroyed_cb->data);
+				ast_free(destroyed_cb);
+			}
+
 			ao2_ref(weakproxy, -1);
 		}
 	}
@@ -796,16 +805,6 @@
 }
 
 
-static void weakproxy_run_callbacks(struct ao2_weakproxy *weakproxy)
-{
-	struct ao2_weakproxy_notification *destroyed_cb;
-
-	while ((destroyed_cb = AST_LIST_REMOVE_HEAD(&weakproxy->destroyed_cb, list))) {
-		destroyed_cb->cb(weakproxy, destroyed_cb->data);
-		ast_free(destroyed_cb);
-	}
-}
-
 void *__ao2_weakproxy_alloc(size_t data_size, ao2_destructor_fn destructor_fn,
 	const char *tag, const char *file, int line, const char *func)
 {
@@ -951,6 +950,7 @@
 {
 	struct astobj2 *weakproxy_internal = INTERNAL_OBJ_CHECK(weakproxy);
 	int ret = -1;
+	int hasobj;
 
 	if (!weakproxy_internal || weakproxy_internal->priv_data.magic != AO2_WEAK) {
 		return -1;
@@ -960,7 +960,8 @@
 		ao2_lock(weakproxy);
 	}
 
-	if (weakproxy_internal->priv_data.weakptr) {
+	hasobj = weakproxy_internal->priv_data.weakptr != NULL;
+	if (hasobj) {
 		struct ao2_weakproxy *weak = weakproxy;
 		struct ao2_weakproxy_notification *sub = ast_calloc(1, sizeof(*sub));
 
@@ -970,15 +971,17 @@
 			AST_LIST_INSERT_HEAD(&weak->destroyed_cb, sub, list);
 			ret = 0;
 		}
-	} else {
-		cb(weakproxy, data);
-		ret = 0;
 	}
 
 	if (!(flags & OBJ_NOLOCK)) {
 		ao2_unlock(weakproxy);
 	}
 
+	if (!hasobj) {
+		cb(weakproxy, data);
+		ret = 0;
+	}
+
 	return ret;
 }
 

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

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ib167622a8a0f873fd73938f7611b2a5914308047
Gerrit-Change-Number: 6733
Gerrit-PatchSet: 2
Gerrit-Owner: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20171012/6f110cdf/attachment-0001.html>