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/
........

Merged revisions 431243 from http://svn.asterisk.org/svn/asterisk/branches/13

git-svn-id: https://origsvn.digium.com/svn/asterisk/trunk@431251 65c4cc65-6c06-0410-ace0-fbb531ad65f3

diff --git a/main/sorcery.c b/main/sorcery.c
index bd919a40b942f7818e4f58e67c239363e3ef5453..50e5e4756653d935a2dc2e7161000d91a302de90 100644 (file)
--- a/main/sorcery.c
+++ b/main/sorcery.c
@@ -1176,12 +1176,6 @@ static int sorcery_wizard_load(void *obj, void *arg, int flags)
        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,22 +1250,31 @@ static int sorcery_object_load(void *obj, void *arg, int flags)
 
        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);
 
+       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 = sorcery_observer_invocation_alloc(type, NULL);
+               struct sorcery_observer_invocation *invocation;
 
-               if (invocation && ast_taskprocessor_push(type->serializer, sorcery_observers_notify_loaded, 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);
-
        return 0;
 }
 

diff --git a/res/res_pjsip.c b/res/res_pjsip.c
index b2b7d63584a224f3f812e11ef01ca0fc82093801..149a2d8be8086e500962325cfd42c67045b4bd9e 100644 (file)
--- a/res/res_pjsip.c
+++ b/res/res_pjsip.c
@@ -3316,7 +3316,11 @@ static int load_module(void)
 
 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;
        }

diff --git a/res/res_pjsip_outbound_registration.c b/res/res_pjsip_outbound_registration.c
index 17f668af745cd1179966a3e0277cfa14b16b1613..e30a32ee34e55212704622d56f077b53c54740af 100644 (file)
--- a/res/res_pjsip_outbound_registration.c
+++ b/res/res_pjsip_outbound_registration.c
@@ -361,37 +361,12 @@ static struct sip_outbound_registration_state *get_state(const char *id)
        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;
 }
 
@@ -1060,11 +1035,16 @@ static int sip_outbound_registration_perform(void *data)
 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));
 
        if (ast_strlen_zero(applied->server_uri)) {
@@ -1089,7 +1069,15 @@ static int sip_outbound_registration_apply(const struct ast_sorcery *sorcery, vo
                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;
        }
 
@@ -1485,9 +1473,11 @@ static void *cli_retrieve_by_id(const char *id)
 
        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;
@@ -1604,14 +1594,66 @@ static void auth_observer(const char *type)
        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_sip_unregister_endpoint_identifier(&line_identifier);
-       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");
@@ -1625,7 +1667,8 @@ static int unload_module(void)
 
 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");
@@ -1660,7 +1703,7 @@ static int load_module(void)
        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;
@@ -1682,14 +1725,15 @@ static int load_module(void)
        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;
 }
@@ -1697,7 +1741,6 @@ static int load_module(void)
 static int reload_module(void)
 {
        ast_sorcery_reload_object(ast_sip_get_sorcery(), "registration");
-       ao2_cleanup(get_registrations());
        return 0;
 }