[asterisk-commits] res pjsip contact: Lock expiration/addition of contacts (asterisk[certified/13.8])

SVN commits to the Asterisk project asterisk-commits at lists.digium.com
Thu Jun 9 14:30:30 CDT 2016


Anonymous Coward #1000019 has submitted this change and it was merged.

Change subject: res_pjsip contact:  Lock expiration/addition of contacts
......................................................................


res_pjsip contact:  Lock expiration/addition of contacts

Contact expiration can occur in several places:  res_pjsip_registrar,
res_pjsip_registrar_expire, and automatically when anyone calls
ast_sip_location_retrieve_aor_contact.  At the same time, res_pjsip_registrar
may also be attempting to renew or add a contact.  Since none of this was locked
it was possible for one thread to be renewing a contact and another thread to
expire it immediately because it was working off of stale data.  This was the
casue of intermittent registration/inbound/nominal/multiple_contacts test
failures.

Now, the new named lock functionality is used to lock the aor during contact
expire and add operations and res_pjsip_registrar_expire now checks the
expiration with the lock held before deleting the contact.

ASTERISK-25885 #close
Reported-by: Josh Colp

Change-Id: I83d413c46a47796f3ab052ca3b349f21cca47059
---
M include/asterisk/res_pjsip.h
M res/res_pjsip/location.c
M res/res_pjsip_registrar.c
M res/res_pjsip_registrar_expire.c
4 files changed, 185 insertions(+), 24 deletions(-)

Approvals:
  George Joseph: Looks good to me, but someone else must approve
  Anonymous Coward #1000019: Verified
  Joshua Colp: Looks good to me, approved



diff --git a/include/asterisk/res_pjsip.h b/include/asterisk/res_pjsip.h
index f56eb7b..acdc972 100644
--- a/include/asterisk/res_pjsip.h
+++ b/include/asterisk/res_pjsip.h
@@ -998,8 +998,26 @@
  *
  * \retval NULL if no contacts available
  * \retval non-NULL if contacts available
+ *
+ * \warning
+ * Since this function prunes expired contacts before returning, it holds a named write
+ * lock on the aor.  If you already hold the lock, call ast_sip_location_retrieve_aor_contacts_nolock instead.
  */
 struct ao2_container *ast_sip_location_retrieve_aor_contacts(const struct ast_sip_aor *aor);
+
+/*!
+ * \brief Retrieve all contacts currently available for an AOR without locking the AOR
+ * \since 13.9.0
+ *
+ * \param aor Pointer to the AOR
+ *
+ * \retval NULL if no contacts available
+ * \retval non-NULL if contacts available
+ *
+ * \warning
+ * This function should only be called if you already hold a named write lock on the aor.
+ */
+struct ao2_container *ast_sip_location_retrieve_aor_contacts_nolock(const struct ast_sip_aor *aor);
 
 /*!
  * \brief Retrieve the first bound contact from a list of AORs
@@ -1051,12 +1069,37 @@
  *
  * \retval -1 failure
  * \retval 0 success
+ *
+ * \warning
+ * This function holds a named write lock on the aor.  If you already hold the lock
+ * you should call ast_sip_location_add_contact_nolock instead.
  */
 int ast_sip_location_add_contact(struct ast_sip_aor *aor, const char *uri,
 	struct timeval expiration_time, const char *path_info, const char *user_agent,
 	struct ast_sip_endpoint *endpoint);
 
 /*!
+ * \brief Add a new contact to an AOR without locking the AOR
+ * \since 13.9.0
+ *
+ * \param aor Pointer to the AOR
+ * \param uri Full contact URI
+ * \param expiration_time Optional expiration time of the contact
+ * \param path_info Path information
+ * \param user_agent User-Agent header from REGISTER request
+ * \param endpoint The endpoint that resulted in the contact being added
+ *
+ * \retval -1 failure
+ * \retval 0 success
+ *
+ * \warning
+ * This function should only be called if you already hold a named write lock on the aor.
+ */
+int ast_sip_location_add_contact_nolock(struct ast_sip_aor *aor, const char *uri,
+	struct timeval expiration_time, const char *path_info, const char *user_agent,
+	struct ast_sip_endpoint *endpoint);
+
+/*!
  * \brief Update a contact
  *
  * \param contact New contact object with details
diff --git a/res/res_pjsip/location.c b/res/res_pjsip/location.c
index 7ff72e4..636de8b 100644
--- a/res/res_pjsip/location.c
+++ b/res/res_pjsip/location.c
@@ -27,6 +27,7 @@
 #include "include/res_pjsip_private.h"
 #include "asterisk/res_pjsip_cli.h"
 #include "asterisk/statsd.h"
+#include "asterisk/named_locks.h"
 
 #include "asterisk/res_pjproject.h"
 
@@ -177,7 +178,7 @@
 	return contact;
 }
 
-struct ao2_container *ast_sip_location_retrieve_aor_contacts(const struct ast_sip_aor *aor)
+struct ao2_container *ast_sip_location_retrieve_aor_contacts_nolock(const struct ast_sip_aor *aor)
 {
 	/* Give enough space for ^ at the beginning and ;@ at the end, since that is our object naming scheme */
 	char regex[strlen(ast_sorcery_object_get_id(aor)) + 4];
@@ -196,6 +197,24 @@
 	if (aor->permanent_contacts) {
 		ao2_callback(aor->permanent_contacts, OBJ_NODATA, contact_link_static, contacts);
 	}
+
+	return contacts;
+}
+
+struct ao2_container *ast_sip_location_retrieve_aor_contacts(const struct ast_sip_aor *aor)
+{
+	struct ao2_container *contacts;
+	struct ast_named_lock *lock;
+
+	lock = ast_named_lock_get(AST_NAMED_LOCK_TYPE_RWLOCK, "aor", ast_sorcery_object_get_id(aor));
+	if (!lock) {
+		return NULL;
+	}
+
+	ao2_wrlock(lock);
+	contacts = ast_sip_location_retrieve_aor_contacts_nolock(aor);
+	ao2_unlock(lock);
+	ast_named_lock_put(lock);
 
 	return contacts;
 }
@@ -283,7 +302,7 @@
 	return ast_sorcery_retrieve_by_id(ast_sip_get_sorcery(), "contact", contact_name);
 }
 
-int ast_sip_location_add_contact(struct ast_sip_aor *aor, const char *uri,
+int ast_sip_location_add_contact_nolock(struct ast_sip_aor *aor, const char *uri,
 		struct timeval expiration_time, const char *path_info, const char *user_agent,
 		struct ast_sip_endpoint *endpoint)
 {
@@ -320,6 +339,27 @@
 	return ast_sorcery_create(ast_sip_get_sorcery(), contact);
 }
 
+int ast_sip_location_add_contact(struct ast_sip_aor *aor, const char *uri,
+		struct timeval expiration_time, const char *path_info, const char *user_agent,
+		struct ast_sip_endpoint *endpoint)
+{
+	int res;
+	struct ast_named_lock *lock;
+
+	lock = ast_named_lock_get(AST_NAMED_LOCK_TYPE_RWLOCK, "aor", ast_sorcery_object_get_id(aor));
+	if (!lock) {
+		return -1;
+	}
+
+	ao2_wrlock(lock);
+	res = ast_sip_location_add_contact_nolock(aor, uri, expiration_time, path_info, user_agent,
+		endpoint);
+	ao2_unlock(lock);
+	ast_named_lock_put(lock);
+
+	return res;
+}
+
 int ast_sip_location_update_contact(struct ast_sip_contact *contact)
 {
 	return ast_sorcery_update(ast_sip_get_sorcery(), contact);
diff --git a/res/res_pjsip_registrar.c b/res/res_pjsip_registrar.c
index 1ee0d5f..851a5f7 100644
--- a/res/res_pjsip_registrar.c
+++ b/res/res_pjsip_registrar.c
@@ -33,6 +33,7 @@
 #include "asterisk/test.h"
 #include "asterisk/taskprocessor.h"
 #include "asterisk/manager.h"
+#include "asterisk/named_locks.h"
 #include "asterisk/res_pjproject.h"
 #include "res_pjsip/include/res_pjsip_private.h"
 
@@ -431,26 +432,20 @@
 	return -1;
 }
 
-static int rx_task(void *data)
+static int rx_task_core(struct rx_task_data *task_data, struct ao2_container *contacts,
+	const char *aor_name)
 {
 	static const pj_str_t USER_AGENT = { "User-Agent", 10 };
-
-	RAII_VAR(struct rx_task_data *, task_data, data, ao2_cleanup);
-	RAII_VAR(struct ao2_container *, contacts, NULL, ao2_cleanup);
 
 	int added = 0, updated = 0, deleted = 0;
 	pjsip_contact_hdr *contact_hdr = NULL;
 	struct registrar_contact_details details = { 0, };
 	pjsip_tx_data *tdata;
-	const char *aor_name = ast_sorcery_object_get_id(task_data->aor);
 	RAII_VAR(struct ast_str *, path_str, NULL, ast_free);
 	struct ast_sip_contact *response_contact;
 	char *user_agent = NULL;
 	pjsip_user_agent_hdr *user_agent_hdr;
 	pjsip_expires_hdr *expires_hdr;
-
-	/* Retrieve the current contacts, we'll need to know whether to update or not */
-	contacts = ast_sip_location_retrieve_aor_contacts(task_data->aor);
 
 	/* So we don't count static contacts against max_contacts we prune them out from the container */
 	ao2_callback(contacts, OBJ_NODATA | OBJ_UNLINK | OBJ_MULTIPLE, registrar_prune_static, NULL);
@@ -522,7 +517,7 @@
 				continue;
 			}
 
-			if (ast_sip_location_add_contact(task_data->aor, contact_uri, ast_tvadd(ast_tvnow(),
+			if (ast_sip_location_add_contact_nolock(task_data->aor, contact_uri, ast_tvadd(ast_tvnow(),
 				ast_samp2tv(expiration, 1)), path_str ? ast_str_buffer(path_str) : NULL,
 					user_agent, task_data->endpoint)) {
 				ast_log(LOG_ERROR, "Unable to bind contact '%s' to AOR '%s'\n",
@@ -564,7 +559,7 @@
 			if (ast_sip_location_update_contact(contact_update)) {
 				ast_log(LOG_ERROR, "Failed to update contact '%s' expiration time to %d seconds.\n",
 					contact->uri, expiration);
-				ast_sorcery_delete(ast_sip_get_sorcery(), contact);
+				ast_sip_location_delete_contact(contact);
 				continue;
 			}
 			ast_debug(3, "Refreshed contact '%s' on AOR '%s' with new expiration of %d seconds\n",
@@ -602,15 +597,14 @@
 		ao2_callback(contacts, OBJ_NODATA | OBJ_MULTIPLE, registrar_delete_contact, NULL);
 	}
 
-	/* Update the contacts as things will probably have changed */
-	ao2_cleanup(contacts);
-
-	contacts = ast_sip_location_retrieve_aor_contacts(task_data->aor);
+	/* Re-retrieve contacts.  Caller will clean up the original container. */
+	contacts = ast_sip_location_retrieve_aor_contacts_nolock(task_data->aor);
 	response_contact = ao2_callback(contacts, 0, NULL, NULL);
 
 	/* Send a response containing all of the contacts (including static) that are present on this AOR */
 	if (ast_sip_create_response(task_data->rdata, 200, response_contact, &tdata) != PJ_SUCCESS) {
 		ao2_cleanup(response_contact);
+		ao2_cleanup(contacts);
 		return PJ_TRUE;
 	}
 	ao2_cleanup(response_contact);
@@ -619,6 +613,7 @@
 	registrar_add_date_header(tdata);
 
 	ao2_callback(contacts, 0, registrar_add_contact, tdata);
+	ao2_cleanup(contacts);
 
 	if ((expires_hdr = pjsip_msg_find_hdr(task_data->rdata->msg_info.msg, PJSIP_H_EXPIRES, NULL))) {
 		expires_hdr = pjsip_expires_hdr_create(tdata->pool, registrar_get_expiration(task_data->aor, NULL, task_data->rdata));
@@ -630,6 +625,38 @@
 	return PJ_TRUE;
 }
 
+static int rx_task(void *data)
+{
+	int res;
+	struct rx_task_data *task_data = data;
+	struct ao2_container *contacts = NULL;
+	struct ast_named_lock *lock;
+	const char *aor_name = ast_sorcery_object_get_id(task_data->aor);
+
+	lock = ast_named_lock_get(AST_NAMED_LOCK_TYPE_RWLOCK, "aor", aor_name);
+	if (!lock) {
+		ao2_cleanup(task_data);
+		return PJ_TRUE;
+	}
+
+	ao2_wrlock(lock);
+	contacts = ast_sip_location_retrieve_aor_contacts_nolock(task_data->aor);
+	if (!contacts) {
+		ao2_unlock(lock);
+		ast_named_lock_put(lock);
+		ao2_cleanup(task_data);
+		return PJ_TRUE;
+	}
+
+	res = rx_task_core(task_data, contacts, aor_name);
+	ao2_cleanup(contacts);
+	ao2_unlock(lock);
+	ast_named_lock_put(lock);
+	ao2_cleanup(task_data);
+
+	return res;
+}
+
 static pj_bool_t registrar_on_rx_request(struct pjsip_rx_data *rdata)
 {
 	RAII_VAR(struct serializer *, ser, NULL, ao2_cleanup);
diff --git a/res/res_pjsip_registrar_expire.c b/res/res_pjsip_registrar_expire.c
index 5b2b350..cde60ed 100644
--- a/res/res_pjsip_registrar_expire.c
+++ b/res/res_pjsip_registrar_expire.c
@@ -29,6 +29,7 @@
 #include "asterisk/res_pjsip.h"
 #include "asterisk/module.h"
 #include "asterisk/sched.h"
+#include "asterisk/named_locks.h"
 
 #define CONTACT_AUTOEXPIRE_BUCKETS 977
 
@@ -121,13 +122,60 @@
 static int contact_expiration_expire(const void *data)
 {
 	struct contact_expiration *expiration = (void *) data;
+	struct ast_sip_contact *fresh_contact;
+	struct ast_named_lock *lock;
+	int reschedule_time = 0;
 
-	expiration->sched = -1;
+	lock = ast_named_lock_get(AST_NAMED_LOCK_TYPE_RWLOCK, "aor", expiration->contact->aor);
+	if (!lock) {
+		/* Uh Oh.  Just expire the contact and don't be nice about it. */
+		expiration->sched = -1;
+		ast_sip_location_delete_contact(expiration->contact);
+		ao2_ref(expiration, -1);
+		return 0;
+	}
 
-	/* This will end up invoking the deleted observer callback, which will perform the unlinking and such */
-	ast_sorcery_delete(ast_sip_get_sorcery(), expiration->contact);
-	ao2_ref(expiration, -1);
-	return 0;
+	/*
+	 * We need to check the expiration again with the aor lock held
+	 * in case another thread is attempting to renew the contact.
+	 */
+	ao2_wrlock(lock);
+
+	fresh_contact = ast_sip_location_retrieve_contact(
+		ast_sorcery_object_get_id(expiration->contact));
+	if (fresh_contact) {
+		int expires;
+
+		expires = ast_tvdiff_ms(fresh_contact->expiration_time, ast_tvnow());
+		if (0 < expires) {
+			/* We need to reschedule for the new expiration time. */
+			reschedule_time = expires;
+		} else {
+			/*
+			 * Contact is expired.
+			 *
+			 * This will end up invoking the deleted observer callback,
+			 * which will perform the unlinking and such.
+			 */
+			expiration->sched = -1;
+			ast_sip_location_delete_contact(fresh_contact);
+			ao2_ref(expiration, -1);
+		}
+		ao2_ref(fresh_contact, -1);
+	} else {
+		/*
+		 * The object no longer exists in sorcery since we
+		 * could not get a fresh copy.
+		 */
+		expiration->sched = -1;
+		ast_sip_location_delete_contact(expiration->contact);
+		ao2_ref(expiration, -1);
+	}
+
+	ao2_unlock(lock);
+	ast_named_lock_put(lock);
+
+	return reschedule_time;
 }
 
 /*! \brief Observer callback for when a contact is created */
@@ -151,7 +199,9 @@
 	ao2_ref(expiration->contact, +1);
 
 	ao2_ref(expiration, +1);
-	if ((expiration->sched = ast_sched_add(sched, expires, contact_expiration_expire, expiration)) < 0) {
+	expiration->sched = ast_sched_add_variable(sched, expires, contact_expiration_expire,
+		expiration, 1);
+	if (expiration->sched < 0) {
 		ao2_ref(expiration, -1);
 		ast_log(LOG_ERROR, "Scheduled expiration for contact '%s' could not be performed, contact may persist past life\n",
 			ast_sorcery_object_get_id(contact));
@@ -174,8 +224,9 @@
 		return;
 	}
 
-	AST_SCHED_REPLACE_UNREF(expiration->sched, sched, expires, contact_expiration_expire,
-		expiration, ao2_cleanup(expiration), ao2_cleanup(expiration), ao2_ref(expiration, +1));
+	AST_SCHED_REPLACE_VARIABLE_UNREF(expiration->sched, sched, expires,
+		contact_expiration_expire, expiration, 1,
+		ao2_cleanup(expiration), ao2_cleanup(expiration), ao2_ref(expiration, +1));
 	ao2_ref(expiration, -1);
 }
 

-- 
To view, visit https://gerrit.asterisk.org/2992
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I83d413c46a47796f3ab052ca3b349f21cca47059
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: certified/13.8
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>



More information about the asterisk-commits mailing list