[Asterisk-code-review] manager: Use separate lock for session event notification. (...asterisk[13])

[Joshua Colp]

Joshua Colp has uploaded this change for review. ( https://gerrit.asterisk.org/c/asterisk/+/11179


Change subject: manager: Use separate lock for session event notification.
......................................................................

manager: Use separate lock for session event notification.

When notifying a manager session that new events were available
the same lock was used that was also held when doing things within
the session (such as sending events out). If the manager session
blocked for a period of time this would cause a back up of messages
in Stasis and would also block any other sessions from receiving
events.

This change adds a separate lock to the manager session which is
strictly used for notifying it that new events are available.

ASTERISK-28350

Change-Id: Ifbcac007faca9ad0231640f5e82a6ca9228f261b
---
M main/manager.c
1 file changed, 31 insertions(+), 18 deletions(-)



  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/79/11179/1

diff --git a/main/manager.c b/main/manager.c
index 5d66b8a..0260436 100644
--- a/main/manager.c
+++ b/main/manager.c
@@ -1593,6 +1593,7 @@
 	time_t noncetime;	/*!< Timer for nonce value expiration */
 	unsigned long oldnonce;	/*!< Stale nonce value */
 	unsigned long nc;	/*!< incremental  nonce counter */
+	ast_mutex_t notify_lock; /*!< Lock for notifying this session of events */
 	AST_LIST_HEAD_NOLOCK(mansession_datastores, ast_datastore) datastores; /*!< Data stores on the session */
 	AST_LIST_ENTRY(mansession_session) list;
 };
@@ -2211,6 +2212,8 @@
 	if (session->blackfilters) {
 		ao2_t_ref(session->blackfilters, -1, "decrement ref for black container, should be last one");
 	}
+
+	ast_mutex_destroy(&session->notify_lock);
 }
 
 /*! \brief Allocate manager session structure and add it to the list of sessions */
@@ -2237,6 +2240,8 @@
 	newsession->send_events = -1;
 	ast_sockaddr_copy(&newsession->addr, addr);
 
+	ast_mutex_init(&newsession->notify_lock);
+
 	sessions = ao2_global_obj_ref(mgr_sessions);
 	if (sessions) {
 		ao2_link(sessions, newsession);
@@ -4176,8 +4181,9 @@
 	}
 	ao2_unlock(s->session);
 
-	/* XXX should this go inside the lock ? */
+	ast_mutex_lock(&s->session->notify_lock);
 	s->session->waiting_thread = pthread_self();	/* let new events wake up this thread */
+	ast_mutex_unlock(&s->session->notify_lock);
 	ast_debug(1, "Starting waiting for an event!\n");
 
 	for (x = 0; x < timeout || timeout < 0; x++) {
@@ -4185,17 +4191,19 @@
 		if (AST_RWLIST_NEXT(s->session->last_ev, eq_next)) {
 			needexit = 1;
 		}
-		/* We can have multiple HTTP session point to the same mansession entry.
-		 * The way we deal with it is not very nice: newcomers kick out the previous
-		 * HTTP session. XXX this needs to be improved.
-		 */
-		if (s->session->waiting_thread != pthread_self()) {
-			needexit = 1;
-		}
 		if (s->session->needdestroy) {
 			needexit = 1;
 		}
 		ao2_unlock(s->session);
+		/* We can have multiple HTTP session point to the same mansession entry.
+		 * The way we deal with it is not very nice: newcomers kick out the previous
+		 * HTTP session. XXX this needs to be improved.
+		 */
+		ast_mutex_lock(&s->session->notify_lock);
+		if (s->session->waiting_thread != pthread_self()) {
+			needexit = 1;
+		}
+		ast_mutex_unlock(&s->session->notify_lock);
 		if (needexit) {
 			break;
 		}
@@ -4209,9 +4217,14 @@
 	}
 	ast_debug(1, "Finished waiting for an event!\n");
 
-	ao2_lock(s->session);
+	ast_mutex_lock(&s->session->notify_lock);
 	if (s->session->waiting_thread == pthread_self()) {
 		struct eventqent *eqe = s->session->last_ev;
+
+		s->session->waiting_thread = AST_PTHREADT_NULL;
+		ast_mutex_unlock(&s->session->notify_lock);
+
+		ao2_lock(s->session);
 		astman_send_response(s, m, "Success", "Waiting for Event completed.");
 		while ((eqe = advance_event(eqe))) {
 			if (((s->session->readperm & eqe->category) == eqe->category)
@@ -4225,11 +4238,11 @@
 			"Event: WaitEventComplete\r\n"
 			"%s"
 			"\r\n", idText);
-		s->session->waiting_thread = AST_PTHREADT_NULL;
+		ao2_unlock(s->session);
 	} else {
+		ast_mutex_unlock(&s->session->notify_lock);
 		ast_debug(1, "Abandoning event request!\n");
 	}
-	ao2_unlock(s->session);
 
 	return 0;
 }
@@ -6580,20 +6593,20 @@
 			}
 		}
 
-		ao2_lock(s->session);
+		ast_mutex_lock(&s->session->notify_lock);
 		if (s->session->pending_event) {
 			s->session->pending_event = 0;
-			ao2_unlock(s->session);
+			ast_mutex_unlock(&s->session->notify_lock);
 			return 0;
 		}
 		s->session->waiting_thread = pthread_self();
-		ao2_unlock(s->session);
+		ast_mutex_unlock(&s->session->notify_lock);
 
 		res = ast_wait_for_input(s->session->fd, timeout);
 
-		ao2_lock(s->session);
+		ast_mutex_lock(&s->session->notify_lock);
 		s->session->waiting_thread = AST_PTHREADT_NULL;
-		ao2_unlock(s->session);
+		ast_mutex_unlock(&s->session->notify_lock);
 	}
 	if (res < 0) {
 		/* If we get a signal from some other thread (typically because
@@ -6985,7 +6998,7 @@
 
 		iter = ao2_iterator_init(sessions, 0);
 		while ((session = ao2_iterator_next(&iter))) {
-			ao2_lock(session);
+			ast_mutex_lock(&session->notify_lock);
 			if (session->waiting_thread != AST_PTHREADT_NULL) {
 				pthread_kill(session->waiting_thread, SIGURG);
 			} else {
@@ -6996,7 +7009,7 @@
 				 */
 				session->pending_event = 1;
 			}
-			ao2_unlock(session);
+			ast_mutex_unlock(&session->notify_lock);
 			unref_mansession(session);
 		}
 		ao2_iterator_destroy(&iter);

-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/11179
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Change-Id: Ifbcac007faca9ad0231640f5e82a6ca9228f261b
Gerrit-Change-Number: 11179
Gerrit-PatchSet: 1
Gerrit-Owner: Joshua Colp <jcolp at digium.com>
Gerrit-MessageType: newchange
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190326/5e4f1a83/attachment.html>