From 88fbe4e91764320e865a8a39b309821697d84b53 Mon Sep 17 00:00:00 2001 From: Richard Mudgett Date: Wed, 28 Jan 2015 04:09:54 +0000 Subject: 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/ git-svn-id: https://origsvn.digium.com/svn/asterisk/branches/13@431243 65c4cc65-6c06-0410-ace0-fbb531ad65f3 --- main/sorcery.c | 25 +++---- res/res_pjsip.c | 6 +- res/res_pjsip_outbound_registration.c | 121 +++++++++++++++++++++++----------- 3 files changed, 101 insertions(+), 51 deletions(-) diff --git a/main/sorcery.c b/main/sorcery.c index bd919a40b..50e5e4756 100644 --- 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 c446dee57..afc383225 100644 --- a/res/res_pjsip.c +++ b/res/res_pjsip.c @@ -3310,7 +3310,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 284520a2d..b8f7112e4 100644 --- a/res/res_pjsip_outbound_registration.c +++ b/res/res_pjsip_outbound_registration.c @@ -345,37 +345,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; } @@ -987,11 +962,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)) { @@ -1008,7 +988,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; } @@ -1404,9 +1392,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; @@ -1523,13 +1513,65 @@ 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_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 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"); @@ -1575,7 +1618,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; @@ -1597,14 +1640,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; } @@ -1612,7 +1656,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; } -- cgit v1.2.3