<p>Friendly Automation <strong>merged</strong> this change.</p><p><a href="https://gerrit.asterisk.org/c/asterisk/+/11180">View Change</a></p><div style="white-space:pre-wrap">Approvals:
George Joseph: Looks good to me, but someone else must approve
Kevin Harwell: Looks good to me, approved
Friendly Automation: Approved for Submit
</div><pre style="font-family: monospace,monospace; white-space: pre-wrap;">manager: Use separate lock for session event notification.<br><br>When notifying a manager session that new events were available<br>the same lock was used that was also held when doing things within<br>the session (such as sending events out). If the manager session<br>blocked for a period of time this would cause a back up of messages<br>in Stasis and would also block any other sessions from receiving<br>events.<br><br>This change adds a separate lock to the manager session which is<br>strictly used for notifying it that new events are available.<br><br>ASTERISK-28350<br><br>Change-Id: Ifbcac007faca9ad0231640f5e82a6ca9228f261b<br>---<br>M main/manager.c<br>1 file changed, 37 insertions(+), 19 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><span>diff --git a/main/manager.c b/main/manager.c</span><br><span>index 9a85580..19b6aa2 100644</span><br><span>--- a/main/manager.c</span><br><span>+++ b/main/manager.c</span><br><span>@@ -1598,6 +1598,7 @@</span><br><span> time_t noncetime; /*!< Timer for nonce value expiration */</span><br><span> unsigned long oldnonce; /*!< Stale nonce value */</span><br><span> unsigned long nc; /*!< incremental nonce counter */</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_mutex_t notify_lock; /*!< Lock for notifying this session of events */</span><br><span> AST_LIST_HEAD_NOLOCK(mansession_datastores, ast_datastore) datastores; /*!< Data stores on the session */</span><br><span> AST_LIST_ENTRY(mansession_session) list;</span><br><span> };</span><br><span>@@ -2211,6 +2212,8 @@</span><br><span> if (session->blackfilters) {</span><br><span> ao2_t_ref(session->blackfilters, -1, "decrement ref for black container, should be last one");</span><br><span> }</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_mutex_destroy(&session->notify_lock);</span><br><span> }</span><br><span> </span><br><span> /*! \brief Allocate manager session structure and add it to the list of sessions */</span><br><span>@@ -2236,6 +2239,8 @@</span><br><span> newsession->send_events = -1;</span><br><span> ast_sockaddr_copy(&newsession->addr, addr);</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+ ast_mutex_init(&newsession->notify_lock);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> sessions = ao2_global_obj_ref(mgr_sessions);</span><br><span> if (sessions) {</span><br><span> ao2_link(sessions, newsession);</span><br><span>@@ -4162,10 +4167,13 @@</span><br><span> /* XXX maybe put an upper bound, or prevent the use of 0 ? */</span><br><span> }</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- ao2_lock(s->session);</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_mutex_lock(&s->session->notify_lock);</span><br><span> if (s->session->waiting_thread != AST_PTHREADT_NULL) {</span><br><span> pthread_kill(s->session->waiting_thread, SIGURG);</span><br><span> }</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_mutex_unlock(&s->session->notify_lock);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ ao2_lock(s->session);</span><br><span> </span><br><span> if (s->session->managerid) { /* AMI-over-HTTP session */</span><br><span> /*</span><br><span>@@ -4188,8 +4196,9 @@</span><br><span> }</span><br><span> ao2_unlock(s->session);</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- /* XXX should this go inside the lock ? */</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_mutex_lock(&s->session->notify_lock);</span><br><span> s->session->waiting_thread = pthread_self(); /* let new events wake up this thread */</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_mutex_unlock(&s->session->notify_lock);</span><br><span> ast_debug(1, "Starting waiting for an event!\n");</span><br><span> </span><br><span> for (x = 0; x < timeout || timeout < 0; x++) {</span><br><span>@@ -4197,17 +4206,19 @@</span><br><span> if (AST_RWLIST_NEXT(s->session->last_ev, eq_next)) {</span><br><span> needexit = 1;</span><br><span> }</span><br><span style="color: hsl(0, 100%, 40%);">- /* We can have multiple HTTP session point to the same mansession entry.</span><br><span style="color: hsl(0, 100%, 40%);">- * The way we deal with it is not very nice: newcomers kick out the previous</span><br><span style="color: hsl(0, 100%, 40%);">- * HTTP session. XXX this needs to be improved.</span><br><span style="color: hsl(0, 100%, 40%);">- */</span><br><span style="color: hsl(0, 100%, 40%);">- if (s->session->waiting_thread != pthread_self()) {</span><br><span style="color: hsl(0, 100%, 40%);">- needexit = 1;</span><br><span style="color: hsl(0, 100%, 40%);">- }</span><br><span> if (s->session->needdestroy) {</span><br><span> needexit = 1;</span><br><span> }</span><br><span> ao2_unlock(s->session);</span><br><span style="color: hsl(120, 100%, 40%);">+ /* We can have multiple HTTP session point to the same mansession entry.</span><br><span style="color: hsl(120, 100%, 40%);">+ * The way we deal with it is not very nice: newcomers kick out the previous</span><br><span style="color: hsl(120, 100%, 40%);">+ * HTTP session. XXX this needs to be improved.</span><br><span style="color: hsl(120, 100%, 40%);">+ */</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_mutex_lock(&s->session->notify_lock);</span><br><span style="color: hsl(120, 100%, 40%);">+ if (s->session->waiting_thread != pthread_self()) {</span><br><span style="color: hsl(120, 100%, 40%);">+ needexit = 1;</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_mutex_unlock(&s->session->notify_lock);</span><br><span> if (needexit) {</span><br><span> break;</span><br><span> }</span><br><span>@@ -4221,9 +4232,14 @@</span><br><span> }</span><br><span> ast_debug(1, "Finished waiting for an event!\n");</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- ao2_lock(s->session);</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_mutex_lock(&s->session->notify_lock);</span><br><span> if (s->session->waiting_thread == pthread_self()) {</span><br><span> struct eventqent *eqe = s->session->last_ev;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ s->session->waiting_thread = AST_PTHREADT_NULL;</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_mutex_unlock(&s->session->notify_lock);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ ao2_lock(s->session);</span><br><span> astman_send_response(s, m, "Success", "Waiting for Event completed.");</span><br><span> while ((eqe = advance_event(eqe))) {</span><br><span> if (((s->session->readperm & eqe->category) == eqe->category)</span><br><span>@@ -4237,11 +4253,11 @@</span><br><span> "Event: WaitEventComplete\r\n"</span><br><span> "%s"</span><br><span> "\r\n", idText);</span><br><span style="color: hsl(0, 100%, 40%);">- s->session->waiting_thread = AST_PTHREADT_NULL;</span><br><span style="color: hsl(120, 100%, 40%);">+ ao2_unlock(s->session);</span><br><span> } else {</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_mutex_unlock(&s->session->notify_lock);</span><br><span> ast_debug(1, "Abandoning event request!\n");</span><br><span> }</span><br><span style="color: hsl(0, 100%, 40%);">- ao2_unlock(s->session);</span><br><span> </span><br><span> return 0;</span><br><span> }</span><br><span>@@ -6618,20 +6634,20 @@</span><br><span> }</span><br><span> }</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- ao2_lock(s->session);</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_mutex_lock(&s->session->notify_lock);</span><br><span> if (s->session->pending_event) {</span><br><span> s->session->pending_event = 0;</span><br><span style="color: hsl(0, 100%, 40%);">- ao2_unlock(s->session);</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_mutex_unlock(&s->session->notify_lock);</span><br><span> return 0;</span><br><span> }</span><br><span> s->session->waiting_thread = pthread_self();</span><br><span style="color: hsl(0, 100%, 40%);">- ao2_unlock(s->session);</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_mutex_unlock(&s->session->notify_lock);</span><br><span> </span><br><span> res = ast_wait_for_input(ast_iostream_get_fd(s->session->stream), timeout);</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- ao2_lock(s->session);</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_mutex_lock(&s->session->notify_lock);</span><br><span> s->session->waiting_thread = AST_PTHREADT_NULL;</span><br><span style="color: hsl(0, 100%, 40%);">- ao2_unlock(s->session);</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_mutex_unlock(&s->session->notify_lock);</span><br><span> }</span><br><span> if (res < 0) {</span><br><span> /* If we get a signal from some other thread (typically because</span><br><span>@@ -7012,7 +7028,7 @@</span><br><span> </span><br><span> iter = ao2_iterator_init(sessions, 0);</span><br><span> while ((session = ao2_iterator_next(&iter))) {</span><br><span style="color: hsl(0, 100%, 40%);">- ao2_lock(session);</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_mutex_lock(&session->notify_lock);</span><br><span> if (session->waiting_thread != AST_PTHREADT_NULL) {</span><br><span> pthread_kill(session->waiting_thread, SIGURG);</span><br><span> } else {</span><br><span>@@ -7023,7 +7039,7 @@</span><br><span> */</span><br><span> session->pending_event = 1;</span><br><span> }</span><br><span style="color: hsl(0, 100%, 40%);">- ao2_unlock(session);</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_mutex_unlock(&session->notify_lock);</span><br><span> unref_mansession(session);</span><br><span> }</span><br><span> ao2_iterator_destroy(&iter);</span><br><span>@@ -7909,9 +7925,11 @@</span><br><span> blastaway = 1;</span><br><span> } else {</span><br><span> ast_debug(1, "Need destroy, but can't do it yet!\n");</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_mutex_lock(&session->notify_lock);</span><br><span> if (session->waiting_thread != AST_PTHREADT_NULL) {</span><br><span> pthread_kill(session->waiting_thread, SIGURG);</span><br><span> }</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_mutex_unlock(&session->notify_lock);</span><br><span> session->inuse--;</span><br><span> }</span><br><span> } else {</span><br><span></span><br></pre><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/11180">change 11180</a>. To unsubscribe, or for help writing mail filters, 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/c/asterisk/+/11180"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 16 </div>
<div style="display:none"> Gerrit-Change-Id: Ifbcac007faca9ad0231640f5e82a6ca9228f261b </div>
<div style="display:none"> Gerrit-Change-Number: 11180 </div>
<div style="display:none"> Gerrit-PatchSet: 2 </div>
<div style="display:none"> Gerrit-Owner: Joshua Colp <jcolp@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation </div>
<div style="display:none"> Gerrit-Reviewer: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-MessageType: merged </div>