[asterisk-commits] mjordan: branch 11 r383840 - in /branches/11: ./ main/cdr.c

[SVN commits to the Asterisk project]

Author: mjordan
Date: Mon Mar 25 20:52:21 2013
New Revision: 383840

URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=383840
Log:
Resolve deadlock between pending CDR and batch CDR locks

r375757 attempted to resolve a race condition between multiple submissions of
CDRs while in batch mode from attempting to destroy the scheduled batch
submission by extending the batch CDR lock. Unfortunately, this causes a
deadlock between the pending CDR lock and the batch CDR lock. This patch
resolves the intent of r375757 by simply providing a new lock that protects
the scheduling of the batches. The original batch CDR lock is kept to protect
manipulation of the batch CDR settings, but has been placed such that it
is not held when the pending lock is held.

Thanks to Chase Venters for providing lock analysis on the issue.

(issue ASTERISK-21162)
Reported by: Chase Venters
........

Merged revisions 383839 from http://svn.asterisk.org/svn/asterisk/branches/1.8

Modified:
    branches/11/   (props changed)
    branches/11/main/cdr.c

Propchange: branches/11/
------------------------------------------------------------------------------
Binary property 'branch-1.8-merged' - no diff available.

Modified: branches/11/main/cdr.c
URL: http://svnview.digium.com/svn/asterisk/branches/11/main/cdr.c?view=diff&rev=383840&r1=383839&r2=383840
==============================================================================
--- branches/11/main/cdr.c (original)
+++ branches/11/main/cdr.c Mon Mar 25 20:52:21 2013
@@ -114,6 +114,8 @@
 
 static int batchsafeshutdown;
 static const int BATCH_SAFE_SHUTDOWN_DEFAULT = 1;
+
+AST_MUTEX_DEFINE_STATIC(cdr_sched_lock);
 
 AST_MUTEX_DEFINE_STATIC(cdr_batch_lock);
 
@@ -1349,17 +1351,24 @@
 {
 	ast_cdr_submit_batch(0);
 	/* manually reschedule from this point in time */
+	ast_mutex_lock(&cdr_sched_lock);
 	cdr_sched = ast_sched_add(sched, batchtime * 1000, submit_scheduled_batch, NULL);
+	ast_mutex_unlock(&cdr_sched_lock);
 	/* returning zero so the scheduler does not automatically reschedule */
 	return 0;
 }
 
+/*! Do not hold the batch lock while calling this function */
 static void submit_unscheduled_batch(void)
 {
+	/* Prevent two deletes from happening at the same time */
+	ast_mutex_lock(&cdr_sched_lock);
 	/* this is okay since we are not being called from within the scheduler */
 	AST_SCHED_DEL(sched, cdr_sched);
 	/* schedule the submission to occur ASAP (1 ms) */
 	cdr_sched = ast_sched_add(sched, 1, submit_scheduled_batch, NULL);
+	ast_mutex_unlock(&cdr_sched_lock);
+
 	/* signal the do_cdr thread to wakeup early and do some work (that lazy thread ;) */
 	ast_mutex_lock(&cdr_pending_lock);
 	ast_cond_signal(&cdr_pending_cond);
@@ -1370,6 +1379,7 @@
 {
 	struct ast_cdr_batch_item *newtail;
 	int curr;
+	int submit_batch = 0;
 
 	if (!cdr)
 		return;
@@ -1416,10 +1426,14 @@
 
 	/* if we have enough stuff to post, then do it */
 	if (curr >= (batchsize - 1)) {
+		submit_batch = 1;
+	}
+	ast_mutex_unlock(&cdr_batch_lock);
+
+	/* Don't call submit_unscheduled_batch with the cdr_batch_lock held */
+	if (submit_batch) {
 		submit_unscheduled_batch();
 	}
-
-	ast_mutex_unlock(&cdr_batch_lock);
 }
 
 static void *do_cdr(void *data)
@@ -1565,7 +1579,9 @@
 	}
 
 	/* don't run the next scheduled CDR posting while reloading */
+	ast_mutex_lock(&cdr_sched_lock);
 	AST_SCHED_DEL(sched, cdr_sched);
+	ast_mutex_unlock(&cdr_sched_lock);
 
 	for (v = ast_variable_browse(config, "general"); v; v = v->next) {
 		if (!strcasecmp(v->name, "enable")) {
@@ -1606,7 +1622,9 @@
 	if (enabled && !batchmode) {
 		ast_log(LOG_NOTICE, "CDR simple logging enabled.\n");
 	} else if (enabled && batchmode) {
+		ast_mutex_lock(&cdr_sched_lock);
 		cdr_sched = ast_sched_add(sched, batchtime * 1000, submit_scheduled_batch, NULL);
+		ast_mutex_unlock(&cdr_sched_lock);
 		ast_log(LOG_NOTICE, "CDR batch mode logging enabled, first of either size %d or time %d seconds.\n", batchsize, batchtime);
 	} else {
 		ast_log(LOG_NOTICE, "CDR logging disabled, data will be lost.\n");
@@ -1618,7 +1636,9 @@
 		ast_cond_init(&cdr_pending_cond, NULL);
 		if (ast_pthread_create_background(&cdr_thread, NULL, do_cdr, NULL) < 0) {
 			ast_log(LOG_ERROR, "Unable to start CDR thread.\n");
+			ast_mutex_lock(&cdr_sched_lock);
 			AST_SCHED_DEL(sched, cdr_sched);
+			ast_mutex_unlock(&cdr_sched_lock);
 		} else {
 			ast_cli_register(&cli_submit);
 			ast_register_atexit(ast_cdr_engine_term);