[Asterisk-code-review] res pjsip contact: Lock expiration/addition of contacts (asterisk[13])
George Joseph
asteriskteam at digium.com
Fri Apr 1 16:15:57 CDT 2016
George Joseph has uploaded a new change for review.
https://gerrit.asterisk.org/2525
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, 76 insertions(+), 7 deletions(-)
git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/25/2525/1
diff --git a/include/asterisk/res_pjsip.h b/include/asterisk/res_pjsip.h
index 50fbc51..c992e9a 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 4284f35..a6d7d51 100644
--- a/res/res_pjsip/location.c
+++ b/res/res_pjsip/location.c
@@ -172,7 +172,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];
@@ -193,6 +193,13 @@
}
return contacts;
+}
+
+struct ao2_container *ast_sip_location_retrieve_aor_contacts(const struct ast_sip_aor *aor)
+{
+ RAII_VAR(struct ast_named_lock *, lock, ast_named_lock_wrlock("aor", ast_sorcery_object_get_id(aor)), ast_named_lock_unlock);
+
+ return ast_sip_location_retrieve_aor_contacts_nolock(aor);
}
void ast_sip_location_retrieve_contact_and_aor_from_list(const char *aor_list, struct ast_sip_aor **aor,
@@ -278,7 +285,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)
{
@@ -315,6 +322,16 @@
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)
+{
+ RAII_VAR(struct ast_named_lock *, lock, ast_named_lock_wrlock("aor", ast_sorcery_object_get_id(aor)), ast_named_lock_unlock);
+
+ return ast_sip_location_add_contact(aor, uri, expiration_time, path_info, user_agent,
+ endpoint);
+}
+
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 fb2b9da..32ca588 100644
--- a/res/res_pjsip_registrar.c
+++ b/res/res_pjsip_registrar.c
@@ -429,9 +429,10 @@
char *user_agent = NULL;
pjsip_user_agent_hdr *user_agent_hdr;
pjsip_expires_hdr *expires_hdr;
+ RAII_VAR(struct ast_named_lock *, lock, ast_named_lock_wrlock("aor", aor_name), ast_named_lock_unlock);
/* Retrieve the current contacts, we'll need to know whether to update or not */
- contacts = ast_sip_location_retrieve_aor_contacts(task_data->aor);
+ contacts = ast_sip_location_retrieve_aor_contacts_nolock(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);
@@ -503,7 +504,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",
@@ -545,7 +546,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",
@@ -587,7 +588,7 @@
/* Update the contacts as things will probably have changed */
ao2_cleanup(contacts);
- contacts = ast_sip_location_retrieve_aor_contacts(task_data->aor);
+ 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 */
diff --git a/res/res_pjsip_registrar_expire.c b/res/res_pjsip_registrar_expire.c
index bdb86e5..1645aec 100644
--- a/res/res_pjsip_registrar_expire.c
+++ b/res/res_pjsip_registrar_expire.c
@@ -41,8 +41,16 @@
static int expire_contact(void *obj, void *arg, int flags)
{
struct ast_sip_contact *contact = obj;
+ struct ast_named_lock *lock = ast_named_lock_wrlock("aor", contact->aor);
- ast_sorcery_delete(ast_sip_get_sorcery(), contact);
+ /*
+ * We need to check the expiration again with the aor lock held
+ * in case another thread is attempting to renew the contact.
+ */
+ if (ast_tvdiff_ms(ast_tvnow(), contact->expiration_time) > 0) {
+ ast_sip_location_delete_contact(contact);
+ }
+ ast_named_lock_unlock(lock);
return 0;
}
--
To view, visit https://gerrit.asterisk.org/2525
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I83d413c46a47796f3ab052ca3b349f21cca47059
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: George Joseph <george.joseph at fairview5.com>
More information about the asterisk-code-review
mailing list