<p>Jenkins2 <strong>merged</strong> this change.</p><p><a href="https://gerrit.asterisk.org/6742">View Change</a></p><div style="white-space:pre-wrap">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
</div><pre style="font-family: monospace,monospace; white-space: pre-wrap;">astobj2: Run weakproxy callbacks outside of lock.<br><br>Copy the list of weakproxy callbacks to temporary memory so they can be<br>run without holding the weakproxy lock.<br><br>Change-Id: Ib167622a8a0f873fd73938f7611b2a5914308047<br>---<br>M include/asterisk/astobj2.h<br>M main/astobj2.c<br>2 files changed, 26 insertions(+), 19 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">diff --git a/include/asterisk/astobj2.h b/include/asterisk/astobj2.h<br>index 33e0224..b118d2e 100644<br>--- a/include/asterisk/astobj2.h<br>+++ b/include/asterisk/astobj2.h<br>@@ -671,6 +671,10 @@<br> * of the cb / data pair. If it was subscribed multiple times it must be<br> * unsubscribed as many times. The OBJ_MULTIPLE flag can be used to remove<br> * matching subscriptions.<br>+ *<br>+ * \note When it's time to run callbacks they are copied to a temporary list so the<br>+ * weakproxy can be unlocked before running. That means it's possible for<br>+ * this function to find nothing before the callback is run in another thread.<br> */<br> int ao2_weakproxy_unsubscribe(void *weakproxy, ao2_weakproxy_notification_cb cb, void *data, int flags);<br> <br>diff --git a/main/astobj2.c b/main/astobj2.c<br>index 1702f51..4b3ade3 100644<br>--- a/main/astobj2.c<br>+++ b/main/astobj2.c<br>@@ -84,8 +84,6 @@<br> AST_LIST_ENTRY(ao2_weakproxy_notification) list;<br> };<br> <br>-static void weakproxy_run_callbacks(struct ao2_weakproxy *weakproxy);<br>-<br> struct ao2_lock_priv {<br> ast_mutex_t lock;<br> };<br>@@ -471,7 +469,7 @@<br> struct astobj2_lockobj *obj_lockobj;<br> int current_value;<br> int ret;<br>- void *weakproxy = NULL;<br>+ struct ao2_weakproxy *weakproxy = NULL;<br> <br> if (obj == NULL) {<br> if (ref_log && user_data) {<br>@@ -500,6 +498,8 @@<br> #endif<br> <br> if (weakproxy) {<br>+ struct ao2_weakproxy cbs;<br>+<br> if (current_value == 1) {<br> /* The only remaining reference is the one owned by the weak object */<br> struct astobj2 *internal_weakproxy;<br>@@ -510,8 +510,9 @@<br> internal_weakproxy->priv_data.weakptr = NULL;<br> obj->priv_data.weakptr = NULL;<br> <br>- /* Notify the subscribers that weakproxy now points to NULL. */<br>- weakproxy_run_callbacks(weakproxy);<br>+ /* transfer list to local copy so callbacks are run with weakproxy unlocked. */<br>+ cbs.destroyed_cb = weakproxy->destroyed_cb;<br>+ AST_LIST_HEAD_INIT_NOLOCK(&weakproxy->destroyed_cb);<br> <br> /* weak is already unlinked from obj so this won't recurse */<br> ao2_ref(user_data, -1);<br>@@ -520,6 +521,14 @@<br> ao2_unlock(weakproxy);<br> <br> if (current_value == 1) {<br>+ struct ao2_weakproxy_notification *destroyed_cb;<br>+<br>+ /* Notify the subscribers that weakproxy now points to NULL. */<br>+ while ((destroyed_cb = AST_LIST_REMOVE_HEAD(&cbs.destroyed_cb, list))) {<br>+ destroyed_cb->cb(weakproxy, destroyed_cb->data);<br>+ ast_free(destroyed_cb);<br>+ }<br>+<br> ao2_ref(weakproxy, -1);<br> }<br> }<br>@@ -820,16 +829,6 @@<br> }<br> <br> <br>-static void weakproxy_run_callbacks(struct ao2_weakproxy *weakproxy)<br>-{<br>- struct ao2_weakproxy_notification *destroyed_cb;<br>-<br>- while ((destroyed_cb = AST_LIST_REMOVE_HEAD(&weakproxy->destroyed_cb, list))) {<br>- destroyed_cb->cb(weakproxy, destroyed_cb->data);<br>- ast_free(destroyed_cb);<br>- }<br>-}<br>-<br> void *__ao2_weakproxy_alloc(size_t data_size, ao2_destructor_fn destructor_fn,<br> const char *tag, const char *file, int line, const char *func)<br> {<br>@@ -975,6 +974,7 @@<br> {<br> struct astobj2 *weakproxy_internal = INTERNAL_OBJ_CHECK(weakproxy);<br> int ret = -1;<br>+ int hasobj;<br> <br> if (!weakproxy_internal || weakproxy_internal->priv_data.magic != AO2_WEAK) {<br> return -1;<br>@@ -984,7 +984,8 @@<br> ao2_lock(weakproxy);<br> }<br> <br>- if (weakproxy_internal->priv_data.weakptr) {<br>+ hasobj = weakproxy_internal->priv_data.weakptr != NULL;<br>+ if (hasobj) {<br> struct ao2_weakproxy *weak = weakproxy;<br> struct ao2_weakproxy_notification *sub = ast_calloc(1, sizeof(*sub));<br> <br>@@ -994,15 +995,17 @@<br> AST_LIST_INSERT_HEAD(&weak->destroyed_cb, sub, list);<br> ret = 0;<br> }<br>- } else {<br>- cb(weakproxy, data);<br>- ret = 0;<br> }<br> <br> if (!(flags & OBJ_NOLOCK)) {<br> ao2_unlock(weakproxy);<br> }<br> <br>+ if (!hasobj) {<br>+ cb(weakproxy, data);<br>+ ret = 0;<br>+ }<br>+<br> return ret;<br> }<br> <br></pre><p>To view, visit <a href="https://gerrit.asterisk.org/6742">change 6742</a>. To unsubscribe, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/6742"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 14 </div>
<div style="display:none"> Gerrit-MessageType: merged </div>
<div style="display:none"> Gerrit-Change-Id: Ib167622a8a0f873fd73938f7611b2a5914308047 </div>
<div style="display:none"> Gerrit-Change-Number: 6742 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Corey Farrell <git@cfware.com> </div>
<div style="display:none"> Gerrit-Reviewer: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Richard Mudgett <rmudgett@digium.com> </div>