<p>George Joseph <strong>merged</strong> this change.</p><p><a href="https://gerrit.asterisk.org/c/asterisk/+/11179">View Change</a></p><div style="white-space:pre-wrap">Approvals:
  George Joseph: Looks good to me, but someone else must approve; Approved for Submit
  Kevin Harwell: Looks good to me, approved

</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 5d66b8a..cb9a713 100644</span><br><span>--- a/main/manager.c</span><br><span>+++ b/main/manager.c</span><br><span>@@ -1593,6 +1593,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>@@ -2237,6 +2240,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>@@ -4150,10 +4155,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>@@ -4176,8 +4184,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>@@ -4185,17 +4194,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>@@ -4209,9 +4220,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>@@ -4225,11 +4241,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>@@ -6580,20 +6596,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(s->session->fd, 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>@@ -6985,7 +7001,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>@@ -6996,7 +7012,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>@@ -7892,9 +7908,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/+/11179">change 11179</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/+/11179"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 13 </div>
<div style="display:none"> Gerrit-Change-Id: Ifbcac007faca9ad0231640f5e82a6ca9228f261b </div>
<div style="display:none"> Gerrit-Change-Number: 11179 </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-Reviewer: Sean Bright <sean.bright@gmail.com> </div>
<div style="display:none"> Gerrit-MessageType: merged </div>