[asterisk-commits] rmudgett: branch 13 r431243 - in /branches/13: main/ res/
SVN commits to the Asterisk project
asterisk-commits at lists.digium.com
Tue Jan 27 22:10:05 CST 2015
Author: rmudgett
Date: Tue Jan 27 22:09:54 2015
New Revision: 431243
URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=431243
Log:
res_pjsip_outbound_registration: Fix reload race condition.
Performing a CLI "module reload" command when there are new pjsip.conf
registration objects defined frequently failed to load them correctly.
What happens is a race condition between res_pjsip pushing its reload into
an asynchronous task processor task and the thread that does the rest of
the reloads when it gets to reloading the res_pjsip_outbound_registration
module. A similar race condition happens between a reload and the CLI/AMI
show registrations commands. The reload updates the current_states
container and the CLI/AMI commands call get_registrations() which builds a
new current_states container.
* Made res_pjsip.c reload_module() use ast_sip_push_task_synchronous()
instead of ast_sip_push_task() to eliminate two threads processing config
reloads at the same time.
* Made get_registrations() not replace the global current_states container
so the CLI/AMI show registrations command cannot interfere with reloading.
You could never add/remove objects in the container without the
possibility of the container being replaced out from under you by
get_registrations().
* Added a registration loaded sorcery instance observer to purge any dead
registration objects since get_registrations() cannot do this job anymore.
The struct ast_sorcery_instance_observer callbacks must be used because
the callback happens inline with the load process. The struct
ast_sorcery_observer callbacks are pushed to a different thread.
* Added some global current_states NULL pointer checks in case the
container disappears because of unload_module().
* Made sorcery's struct ast_sorcery_instance_observer.object_type_loaded
callbacks guaranteed to be called before any struct
ast_sorcery_observer.loaded callbacks will be called.
* Moved the check for non-reloadable objects to before the sorcery
instance loading callbacks happen to short circuit unnecessary work.
Previously with non-reloadable objects, the sorcery instance
loading/loaded callbacks would always happen, the individual wizard
loading/loaded would be prevented, and the non-reloadable type logging
message would be logged for each associated wizard.
ASTERISK-24729 #close
Review: https://reviewboard.asterisk.org/r/4381/
Modified:
branches/13/main/sorcery.c
branches/13/res/res_pjsip.c
branches/13/res/res_pjsip_outbound_registration.c
Modified: branches/13/main/sorcery.c
URL: http://svnview.digium.com/svn/asterisk/branches/13/main/sorcery.c?view=diff&rev=431243&r1=431242&r2=431243
==============================================================================
--- branches/13/main/sorcery.c (original)
+++ branches/13/main/sorcery.c Tue Jan 27 22:09:54 2015
@@ -1176,12 +1176,6 @@
struct sorcery_load_details *details = arg;
void (*load)(void *data, const struct ast_sorcery *sorcery, const char *type);
- if (details->reload && !sorcery_reloadable(details->sorcery, details->type)) {
- ast_log(LOG_NOTICE, "Type '%s' is not reloadable, "
- "maintaining previous values\n", details->type);
- return 0;
- }
-
load = !details->reload ? wizard->wizard->callbacks.load : wizard->wizard->callbacks.reload;
if (load) {
@@ -1256,21 +1250,30 @@
details->type = type->name;
+ if (details->reload && !sorcery_reloadable(details->sorcery, details->type)) {
+ ast_log(LOG_NOTICE, "Type '%s' is not reloadable, maintaining previous values\n",
+ details->type);
+ return 0;
+ }
+
NOTIFY_INSTANCE_OBSERVERS(details->sorcery->observers, object_type_loading,
details->sorcery->module_name, details->sorcery, type->name, details->reload);
ao2_callback(type->wizards, OBJ_NODATA, sorcery_wizard_load, details);
- if (ao2_container_count(type->observers)) {
- struct sorcery_observer_invocation *invocation = sorcery_observer_invocation_alloc(type, NULL);
-
- if (invocation && ast_taskprocessor_push(type->serializer, sorcery_observers_notify_loaded, invocation)) {
- ao2_cleanup(invocation);
- }
- }
-
NOTIFY_INSTANCE_OBSERVERS(details->sorcery->observers, object_type_loaded,
details->sorcery->module_name, details->sorcery, type->name, details->reload);
+
+ if (ao2_container_count(type->observers)) {
+ struct sorcery_observer_invocation *invocation;
+
+ invocation = sorcery_observer_invocation_alloc(type, NULL);
+ if (invocation
+ && ast_taskprocessor_push(type->serializer, sorcery_observers_notify_loaded,
+ invocation)) {
+ ao2_cleanup(invocation);
+ }
+ }
return 0;
}
Modified: branches/13/res/res_pjsip.c
URL: http://svnview.digium.com/svn/asterisk/branches/13/res/res_pjsip.c?view=diff&rev=431243&r1=431242&r2=431243
==============================================================================
--- branches/13/res/res_pjsip.c (original)
+++ branches/13/res/res_pjsip.c Tue Jan 27 22:09:54 2015
@@ -3310,7 +3310,11 @@
static int reload_module(void)
{
- if (ast_sip_push_task(NULL, reload_configuration_task, NULL)) {
+ /*
+ * We must wait for the reload to complete so multiple
+ * reloads cannot happen at the same time.
+ */
+ if (ast_sip_push_task_synchronous(NULL, reload_configuration_task, NULL)) {
ast_log(LOG_WARNING, "Failed to reload PJSIP\n");
return -1;
}
Modified: branches/13/res/res_pjsip_outbound_registration.c
URL: http://svnview.digium.com/svn/asterisk/branches/13/res/res_pjsip_outbound_registration.c?view=diff&rev=431243&r1=431242&r2=431243
==============================================================================
--- branches/13/res/res_pjsip_outbound_registration.c (original)
+++ branches/13/res/res_pjsip_outbound_registration.c Tue Jan 27 22:09:54 2015
@@ -345,37 +345,12 @@
return states ? ao2_find(states, id, OBJ_SEARCH_KEY) : NULL;
}
-static int registration_state_add(void *obj, void *arg, int flags)
-{
- struct sip_outbound_registration_state *state =
- get_state(ast_sorcery_object_get_id(obj));
-
- if (state) {
- ao2_link(arg, state);
- ao2_ref(state, -1);
- }
-
- return 0;
-}
-
static struct ao2_container *get_registrations(void)
{
- RAII_VAR(struct ao2_container *, new_states, NULL, ao2_cleanup);
struct ao2_container *registrations = ast_sorcery_retrieve_by_fields(
ast_sip_get_sorcery(), "registration",
AST_RETRIEVE_FLAG_MULTIPLE | AST_RETRIEVE_FLAG_ALL, NULL);
- if (!(new_states = ao2_container_alloc(DEFAULT_STATE_BUCKETS,
- registration_state_hash, registration_state_cmp))) {
- ast_log(LOG_ERROR, "Unable to allocate registration states container\n");
- return NULL;
- }
-
- if (registrations && ao2_container_count(registrations)) {
- ao2_callback(registrations, OBJ_NODATA, registration_state_add, new_states);
- }
-
- ao2_global_obj_replace_unref(current_states, new_states);
return registrations;
}
@@ -987,10 +962,15 @@
static int sip_outbound_registration_apply(const struct ast_sorcery *sorcery, void *obj)
{
RAII_VAR(struct ao2_container *, states, ao2_global_obj_ref(current_states), ao2_cleanup);
- RAII_VAR(struct sip_outbound_registration_state *, state,
- ao2_find(states, ast_sorcery_object_get_id(obj), OBJ_SEARCH_KEY), ao2_cleanup);
+ RAII_VAR(struct sip_outbound_registration_state *, state, NULL, ao2_cleanup);
RAII_VAR(struct sip_outbound_registration_state *, new_state, NULL, ao2_cleanup);
struct sip_outbound_registration *applied = obj;
+
+ if (!states) {
+ /* Global container has gone. Likely shutting down. */
+ return -1;
+ }
+ state = ao2_find(states, ast_sorcery_object_get_id(applied), OBJ_SEARCH_KEY);
ast_debug(4, "Applying configuration to outbound registration '%s'\n", ast_sorcery_object_get_id(applied));
@@ -1008,7 +988,15 @@
ast_debug(4,
"No change between old configuration and new configuration on outbound registration '%s'. Using previous state\n",
ast_sorcery_object_get_id(applied));
+
+ /*
+ * This is OK to replace without relinking the state in the
+ * current_states container since state->registration and
+ * applied have the same key.
+ */
+ ao2_lock(states);
ao2_replace(state->registration, applied);
+ ao2_unlock(states);
return 0;
}
@@ -1404,9 +1392,11 @@
if (!obj) {
/* if the object no longer exists then remove its state */
- ao2_find((states = ao2_global_obj_ref(current_states)),
- id, OBJ_SEARCH_KEY | OBJ_UNLINK | OBJ_NODATA);
- ao2_ref(states, -1);
+ states = ao2_global_obj_ref(current_states);
+ if (states) {
+ ao2_find(states, id, OBJ_SEARCH_KEY | OBJ_UNLINK | OBJ_NODATA);
+ ao2_ref(states, -1);
+ }
}
return obj;
@@ -1523,13 +1513,65 @@
ao2_cleanup(regs);
}
-const struct ast_sorcery_observer observer_callbacks = {
+static const struct ast_sorcery_observer observer_callbacks_auth = {
.loaded = auth_observer,
};
+static int check_state(void *obj, void *arg, int flags)
+{
+ struct sip_outbound_registration_state *state = obj;
+ struct sip_outbound_registration *registration;
+
+ registration = ast_sorcery_retrieve_by_id(ast_sip_get_sorcery(), "registration",
+ ast_sorcery_object_get_id(state->registration));
+ if (!registration) {
+ /* This is a dead registration */
+ return CMP_MATCH;
+ }
+
+ ao2_ref(registration, -1);
+ return 0;
+}
+
+/*!
+ * \internal
+ * \brief Observer to purge dead registration states.
+ *
+ * \param name Module name owning the sorcery instance.
+ * \param sorcery Instance being observed.
+ * \param object_type Name of object being observed.
+ * \param reloaded Non-zero if the object is being reloaded.
+ *
+ * \return Nothing
+ */
+static void registration_loaded_observer(const char *name, const struct ast_sorcery *sorcery, const char *object_type, int reloaded)
+{
+ struct ao2_container *states;
+
+ if (strcmp(object_type, "registration")) {
+ /* Not interested */
+ return;
+ }
+
+ states = ao2_global_obj_ref(current_states);
+ if (!states) {
+ /* Global container has gone. Likely shutting down. */
+ return;
+ }
+
+ /* Now to purge dead registrations. */
+ ao2_callback(states, OBJ_UNLINK | OBJ_NODATA | OBJ_MULTIPLE, check_state, NULL);
+ ao2_ref(states, -1);
+}
+
+static const struct ast_sorcery_instance_observer observer_callbacks_registrations = {
+ .object_type_loaded = registration_loaded_observer,
+};
+
static int unload_module(void)
{
- ast_sorcery_observer_remove(ast_sip_get_sorcery(), "auth", &observer_callbacks);
+ ast_sorcery_observer_remove(ast_sip_get_sorcery(), "auth", &observer_callbacks_auth);
+ ast_sorcery_instance_observer_remove(ast_sip_get_sorcery(), &observer_callbacks_registrations);
ast_cli_unregister_multiple(cli_outbound_registration, ARRAY_LEN(cli_outbound_registration));
ast_sip_unregister_cli_formatter(cli_formatter);
ast_manager_unregister("PJSIPShowRegistrationsOutbound");
@@ -1543,7 +1585,8 @@
static int load_module(void)
{
- struct ao2_container *registrations, *new_states;
+ struct ao2_container *new_states;
+
CHECK_PJSIP_MODULE_LOADED();
ast_sorcery_apply_config(ast_sip_get_sorcery(), "res_pjsip_outbound_registration");
@@ -1575,7 +1618,7 @@
if (!cli_formatter) {
ast_log(LOG_ERROR, "Unable to allocate memory for cli formatter\n");
unload_module();
- return -1;
+ return AST_MODULE_LOAD_FAILURE;
}
cli_formatter->name = "registration";
cli_formatter->print_header = cli_print_header;
@@ -1597,14 +1640,15 @@
ao2_global_obj_replace_unref(current_states, new_states);
ao2_ref(new_states, -1);
- ast_sorcery_reload_object(ast_sip_get_sorcery(), "registration");
- if (!(registrations = get_registrations())) {
+ if (ast_sorcery_instance_observer_add(ast_sip_get_sorcery(),
+ &observer_callbacks_registrations)) {
unload_module();
return AST_MODULE_LOAD_FAILURE;
}
- ao2_ref(registrations, -1);
-
- ast_sorcery_observer_add(ast_sip_get_sorcery(), "auth", &observer_callbacks);
+
+ ast_sorcery_load_object(ast_sip_get_sorcery(), "registration");
+
+ ast_sorcery_observer_add(ast_sip_get_sorcery(), "auth", &observer_callbacks_auth);
return AST_MODULE_LOAD_SUCCESS;
}
@@ -1612,7 +1656,6 @@
static int reload_module(void)
{
ast_sorcery_reload_object(ast_sip_get_sorcery(), "registration");
- ao2_cleanup(get_registrations());
return 0;
}
More information about the asterisk-commits
mailing list