[Asterisk-code-review] main/cdr.c: Alleviates CDR deadlock (asterisk[15])

[Matthew Fredrickson]

Matthew Fredrickson has uploaded this change for review. ( https://gerrit.asterisk.org/9270


Change subject: main/cdr.c: Alleviates CDR deadlock
......................................................................

main/cdr.c: Alleviates CDR deadlock

There is a rare case (do to the infrequent timing involved) where
CDR submission threads in batch mode can deadlock with a currently
running CDR batch process.  This patch should remove the need for
holding the lock in the scheduler and should clean a few code
paths up that inconsistently submitted new work to the CDR batch
processor.  This is for ASTERISK-27909.

Change-Id: I6333e865db7c593c102c2fd948cecdb96481974d
Reported-by: Denis Lebedev
---
M main/cdr.c
1 file changed, 20 insertions(+), 13 deletions(-)



  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/70/9270/1

diff --git a/main/cdr.c b/main/cdr.c
index b0a48e1..9e44f62 100644
--- a/main/cdr.c
+++ b/main/cdr.c
@@ -3784,6 +3784,7 @@
 static int submit_scheduled_batch(const void *data)
 {
 	struct module_config *mod_cfg;
+	int nextms = 0;
 
 	cdr_submit_batch(0);
 
@@ -3793,24 +3794,29 @@
 	}
 
 	/* manually reschedule from this point in time */
-	ast_mutex_lock(&cdr_sched_lock);
-	cdr_sched = ast_sched_add(sched, mod_cfg->general->batch_settings.time * 1000, submit_scheduled_batch, NULL);
-	ast_mutex_unlock(&cdr_sched_lock);
+	nextms = mod_cfg->general->batch_settings.time * 1000;
+
+	if (!nextms) {
+		/* I hate doing this, but previously if you set the batch time to 0, it would continuously
+		   resubmit batches to the scheduler.  IMHO that seems buggy, but I'd rather not break it
+		   if someone was relying on that behavior */
+		nextms = 1;
+	}
 
 	ao2_cleanup(mod_cfg);
 	/* returning zero so the scheduler does not automatically reschedule */
-	return 0;
+	return nextms;
 }
 
 /*! Do not hold the batch lock while calling this function */
-static void submit_unscheduled_batch(void)
+static void start_batch_mode(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);
+	cdr_sched = ast_sched_add_variable(sched, 1, submit_scheduled_batch, NULL, 1);
 	ast_mutex_unlock(&cdr_sched_lock);
 
 	/* signal the do_cdr thread to wakeup early and do some work (that lazy thread ;) */
@@ -3875,9 +3881,9 @@
 	}
 	ast_mutex_unlock(&cdr_batch_lock);
 
-	/* Don't call submit_unscheduled_batch with the cdr_batch_lock held */
+	/* Don't call start_batch with cdr_batch_lock held */
 	if (submit_batch) {
-		submit_unscheduled_batch();
+		start_batch_mode();
 	}
 }
 
@@ -3890,8 +3896,8 @@
 	for (;;) {
 		struct timeval now;
 		schedms = ast_sched_wait(sched);
-		/* this shouldn't happen, but provide a 1 second default just in case */
-		if (schedms <= 0)
+		/* this only can happen if someone sets there batch time to 0 or does 'cdr submit' from the CLI back to back quickly */
+		if (schedms < 0)
 			schedms = 1000;
 		now = ast_tvadd(ast_tvnow(), ast_samp2tv(schedms, 1000));
 		timeout.tv_sec = now.tv_sec;
@@ -4219,7 +4225,7 @@
 	} else if (!ast_test_flag(&mod_cfg->general->settings, CDR_BATCHMODE)) {
 		ast_cli(a->fd, "Cannot submit CDR batch: batch mode not enabled.\n");
 	} else {
-		submit_unscheduled_batch();
+		start_batch_mode();
 		ast_cli(a->fd, "Submitted CDRs to backend engines for processing.  This may take a while.\n");
 	}
 	ao2_cleanup(mod_cfg);
@@ -4412,9 +4418,10 @@
 		}
 	}
 
+	/* Start the batching process */
+	start_batch_mode();
+
 	/* Kill the currently scheduled item */
-	AST_SCHED_DEL(sched, cdr_sched);
-	cdr_sched = ast_sched_add(sched, config->batch_settings.time * 1000, submit_scheduled_batch, NULL);
 	ast_log(LOG_NOTICE, "CDR batch mode logging enabled, first of either size %u or time %u seconds.\n",
 			config->batch_settings.size, config->batch_settings.time);
 }

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

Gerrit-Project: asterisk
Gerrit-Branch: 15
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6333e865db7c593c102c2fd948cecdb96481974d
Gerrit-Change-Number: 9270
Gerrit-PatchSet: 1
Gerrit-Owner: Matthew Fredrickson <creslin at digium.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180621/3ceff386/attachment-0001.html>