[Asterisk-code-review] res sorcery memory cache.c: Shutdown in a less crash potenti... (asterisk[master])

[Joshua Colp]

Joshua Colp has submitted this change and it was merged.

Change subject: res_sorcery_memory_cache.c: Shutdown in a less crash potential order.
......................................................................


res_sorcery_memory_cache.c: Shutdown in a less crash potential order.

Basically you should shutdown in the opposite order of how you setup since
later setup pieces likely depend on earlier setup pieces.  e.g.,
Registering your external API with the rest of the system should be the
last thing setup and the first thing unregistered during shutdown.

Change-Id: I5715765b723100c8d3c2642e9e72cc7ad5ad115e
---
M res/res_sorcery_memory_cache.c
1 file changed, 29 insertions(+), 24 deletions(-)

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



diff --git a/res/res_sorcery_memory_cache.c b/res/res_sorcery_memory_cache.c
index 0a33ae4..53fa3b9 100644
--- a/res/res_sorcery_memory_cache.c
+++ b/res/res_sorcery_memory_cache.c
@@ -2482,22 +2482,6 @@
 
 static int unload_module(void)
 {
-	if (sched) {
-		ast_sched_context_destroy(sched);
-		sched = NULL;
-	}
-
-	ao2_cleanup(caches);
-
-	ast_sorcery_wizard_unregister(&memory_cache_object_wizard);
-
-	ast_cli_unregister_multiple(cli_memory_cache, ARRAY_LEN(cli_memory_cache));
-
-	ast_manager_unregister("SorceryMemoryCacheExpireObject");
-	ast_manager_unregister("SorceryMemoryCacheExpire");
-	ast_manager_unregister("SorceryMemoryCacheStaleObject");
-	ast_manager_unregister("SorceryMemoryCacheStale");
-
 	AST_TEST_UNREGISTER(open_with_valid_options);
 	AST_TEST_UNREGISTER(open_with_invalid_options);
 	AST_TEST_UNREGISTER(create_and_retrieve);
@@ -2507,12 +2491,41 @@
 	AST_TEST_UNREGISTER(expiration);
 	AST_TEST_UNREGISTER(stale);
 
+	ast_manager_unregister("SorceryMemoryCacheExpireObject");
+	ast_manager_unregister("SorceryMemoryCacheExpire");
+	ast_manager_unregister("SorceryMemoryCacheStaleObject");
+	ast_manager_unregister("SorceryMemoryCacheStale");
+
+	ast_cli_unregister_multiple(cli_memory_cache, ARRAY_LEN(cli_memory_cache));
+
+	ast_sorcery_wizard_unregister(&memory_cache_object_wizard);
+
+	/*
+	 * XXX There is the potential to leak memory if there are pending
+	 * next-cache-expiration and stale-cache-update tasks in the scheduler.
+	 */
+	if (sched) {
+		ast_sched_context_destroy(sched);
+		sched = NULL;
+	}
+
+	ao2_cleanup(caches);
+	caches = NULL;
+
 	return 0;
 }
 
 static int load_module(void)
 {
 	int res;
+
+	caches = ao2_container_alloc(CACHES_CONTAINER_BUCKET_SIZE, sorcery_memory_cache_hash,
+		sorcery_memory_cache_cmp);
+	if (!caches) {
+		ast_log(LOG_ERROR, "Failed to create container for configured caches\n");
+		unload_module();
+		return AST_MODULE_LOAD_DECLINE;
+	}
 
 	sched = ast_sched_context_create();
 	if (!sched) {
@@ -2523,14 +2536,6 @@
 
 	if (ast_sched_start_thread(sched)) {
 		ast_log(LOG_ERROR, "Failed to create scheduler thread for cache management\n");
-		unload_module();
-		return AST_MODULE_LOAD_DECLINE;
-	}
-
-	caches = ao2_container_alloc(CACHES_CONTAINER_BUCKET_SIZE, sorcery_memory_cache_hash,
-		sorcery_memory_cache_cmp);
-	if (!caches) {
-		ast_log(LOG_ERROR, "Failed to create container for configured caches\n");
 		unload_module();
 		return AST_MODULE_LOAD_DECLINE;
 	}

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

Gerrit-MessageType: merged
Gerrit-Change-Id: I5715765b723100c8d3c2642e9e72cc7ad5ad115e
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>