[Asterisk-code-review] manager: Use separate lock for session event notification. (...asterisk[16])
Friendly Automation
asteriskteam at digium.com
Wed Mar 27 17:42:57 CDT 2019
Friendly Automation has submitted this change and it was merged. ( https://gerrit.asterisk.org/c/asterisk/+/11180 )
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, 37 insertions(+), 19 deletions(-)
Approvals:
George Joseph: Looks good to me, but someone else must approve
Kevin Harwell: Looks good to me, approved
Friendly Automation: Approved for Submit
diff --git a/main/manager.c b/main/manager.c
index 9a85580..19b6aa2 100644
--- a/main/manager.c
+++ b/main/manager.c
@@ -1598,6 +1598,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 */
@@ -2236,6 +2239,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);
@@ -4162,10 +4167,13 @@
/* XXX maybe put an upper bound, or prevent the use of 0 ? */
}
- ao2_lock(s->session);
+ ast_mutex_lock(&s->session->notify_lock);
if (s->session->waiting_thread != AST_PTHREADT_NULL) {
pthread_kill(s->session->waiting_thread, SIGURG);
}
+ ast_mutex_unlock(&s->session->notify_lock);
+
+ ao2_lock(s->session);
if (s->session->managerid) { /* AMI-over-HTTP session */
/*
@@ -4188,8 +4196,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++) {
@@ -4197,17 +4206,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;
}
@@ -4221,9 +4232,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)
@@ -4237,11 +4253,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;
}
@@ -6618,20 +6634,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(ast_iostream_get_fd(s->session->stream), 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
@@ -7012,7 +7028,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 {
@@ -7023,7 +7039,7 @@
*/
session->pending_event = 1;
}
- ao2_unlock(session);
+ ast_mutex_unlock(&session->notify_lock);
unref_mansession(session);
}
ao2_iterator_destroy(&iter);
@@ -7909,9 +7925,11 @@
blastaway = 1;
} else {
ast_debug(1, "Need destroy, but can't do it yet!\n");
+ ast_mutex_lock(&session->notify_lock);
if (session->waiting_thread != AST_PTHREADT_NULL) {
pthread_kill(session->waiting_thread, SIGURG);
}
+ ast_mutex_unlock(&session->notify_lock);
session->inuse--;
}
} else {
--
To view, visit https://gerrit.asterisk.org/c/asterisk/+/11180
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: 16
Gerrit-Change-Id: Ifbcac007faca9ad0231640f5e82a6ca9228f261b
Gerrit-Change-Number: 11180
Gerrit-PatchSet: 2
Gerrit-Owner: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190327/c711a8b4/attachment-0001.html>
More information about the asterisk-code-review
mailing list