[Asterisk-code-review] res musiconhold: Add kill escalation delay, kill method to ... (asterisk[13])

Jenkins2 asteriskteam at digium.com
Wed Jul 12 05:55:38 CDT 2017


Jenkins2 has submitted this change and it was merged. ( https://gerrit.asterisk.org/5984 )

Change subject: res_musiconhold:  Add kill_escalation_delay, kill_method to class
......................................................................

res_musiconhold:  Add kill_escalation_delay, kill_method to class

By default, when res_musiconhold reloads or unloads, it sends a HUP
signal to custom applications (and all descendants), waits 100ms,
then sends a TERM signal, waits 100ms, then finally sends a KILL
signal.  An application which is interacting with an external
device and/or spawns children of its own may not be able to exit
cleanly in the default times, expecially if sent a KILL signal, or
if it's children are getting signals directly from
res_musiconhoild.

* To allow extra time, the 'kill_escalation_delay'
  class option can be used to set the number of milliseconds
  res_musiconhold waits before escalating kill signals, with the
  default being the current 100ms.

* To control to whom the signals are sent, the "kill_method" class
  option can be set to "process_group" (the default, existing
  behavior), which sends signals to the application and its
  descendants directly, or "process" which sends signals only to the
  application itself.

Change-Id: Iff70a1a9405685a9021a68416830c0db5158603b
---
M CHANGES
M configs/samples/musiconhold.conf.sample
M res/res_musiconhold.c
3 files changed, 123 insertions(+), 49 deletions(-)

Approvals:
  Richard Mudgett: Looks good to me, but someone else must approve
  Benjamin Keith Ford: Looks good to me, but someone else must approve
  Joshua Colp: Looks good to me, approved; Verified
  Jenkins2: Approved for Submit



diff --git a/CHANGES b/CHANGES
index c3a2d0a..c7d801d 100644
--- a/CHANGES
+++ b/CHANGES
@@ -9,6 +9,26 @@
 ==============================================================================
 
 ------------------------------------------------------------------------------
+--- Functionality changes from Asterisk 13.17.0 to Asterisk 13.18.0 ----------
+------------------------------------------------------------------------------
+
+res_musiconhold
+------------------
+ * By default, when res_musiconhold reloads or unloads, it sends a HUP signal
+   to custom applications (and all descendants), waits 100ms, then sends a
+   TERM signal, waits 100ms, then finally sends a KILL signal.  An application
+   which is interacting with an external device and/or spawns children of its
+   own may not be able to exit cleanly in the default times, expecially if sent
+   a KILL signal, or if it's children are getting signals directly from
+   res_musiconhoild.  To allow extra time, the 'kill_escalation_delay'
+   class option can be used to set the number of milliseconds res_musiconhold
+   waits before escalating kill signals, with the default being the current
+   100ms.  To control to whom the signals are sent, the "kill_method"
+   class option can be set to "process_group" (the default, existing behavior),
+   which sends signals to the application and its descendants directly, or
+   "process" which sends signals only to the application itself.
+
+------------------------------------------------------------------------------
 --- Functionality changes from Asterisk 13.16.0 to Asterisk 13.17.0 ----------
 ------------------------------------------------------------------------------
 
diff --git a/configs/samples/musiconhold.conf.sample b/configs/samples/musiconhold.conf.sample
index 8b2202d..67570ee 100644
--- a/configs/samples/musiconhold.conf.sample
+++ b/configs/samples/musiconhold.conf.sample
@@ -91,3 +91,26 @@
 ;mode=custom
 ;directory=/var/lib/asterisk/mohmp3
 ;application=/site/sw/bin/madplay -Q -o raw:- --mono -R 8000 -a -12
+
+; By default, when res_musiconhold reloads or unloads, it sends a HUP signal
+; to custom applications (and all descendants), waits 100ms, then sends a
+; TERM signal, waits 100ms, then finally sends a KILL signal.  An application
+; which is interacting with an external device and/or spawns children of its
+; own may not be able to exit cleanly in the default times, expecially if sent
+; a KILL signal, or if it's children are getting signals directly from
+; res_musiconhoild.  To allow extra time, the 'kill_escalation_delay'
+; class option can be used to set the number of milliseconds res_musiconhold
+; waits before escalating kill signals, with the default being the current
+; 100ms.  To control to whom the signals are sent, the "kill_method"
+; class option can be set to "process_group" (the default, existing behavior),
+; which sends signals to the application and its descendants directly, or
+; "process" which sends signals only to the application itself.
+
+;[sox_from_device]
+;mode=custom
+;directory=/var/lib/asterisk/mohmp3
+;application=/usr/bin/sox -q -t alsa -c 2 -r 48000 hw:1 -c 1 -r 8000 -t raw -s -
+; Wait 500ms before escalating kill signals
+;kill_escalation_delay=500
+; Send signals to just the child process instead of all descendants
+;kill_method=process
diff --git a/res/res_musiconhold.c b/res/res_musiconhold.c
index c52c964..d791516 100644
--- a/res/res_musiconhold.c
+++ b/res/res_musiconhold.c
@@ -158,6 +158,11 @@
 
 static struct ast_flags global_flags[1] = {{0}};        /*!< global MOH_ flags */
 
+enum kill_methods {
+	KILL_METHOD_PROCESS_GROUP = 0,
+	KILL_METHOD_PROCESS
+};
+
 struct mohclass {
 	char name[MAX_MUSICCLASS];
 	char dir[256];
@@ -178,6 +183,10 @@
 	int pid;
 	time_t start;
 	pthread_t thread;
+	/*! Millisecond delay between kill attempts */
+	size_t kill_delay;
+	/*! Kill method */
+	enum kill_methods kill_method;
 	/*! Source of audio */
 	int srcfd;
 	/*! Generic timer */
@@ -678,6 +687,51 @@
 	return fds[0];
 }
 
+static int killer(pid_t pid, int signum, enum kill_methods kill_method)
+{
+	switch (kill_method) {
+	case KILL_METHOD_PROCESS_GROUP:
+		return killpg(pid, signum);
+	case KILL_METHOD_PROCESS:
+		return kill(pid, signum);
+	}
+
+	return -1;
+}
+
+static void killpid(int pid, size_t delay, enum kill_methods kill_method)
+{
+	if (killer(pid, SIGHUP, kill_method) < 0) {
+		if (errno == ESRCH) {
+			return;
+		}
+		ast_log(LOG_WARNING, "Unable to send a SIGHUP to MOH process '%d'?!!: %s\n", pid, strerror(errno));
+	} else {
+		ast_debug(1, "Sent HUP to pid %d%s\n", pid,
+			kill_method == KILL_METHOD_PROCESS_GROUP ? " and all children" : " only");
+	}
+	usleep(delay);
+	if (killer(pid, SIGTERM, kill_method) < 0) {
+		if (errno == ESRCH) {
+			return;
+		}
+		ast_log(LOG_WARNING, "Unable to terminate MOH process '%d'?!!: %s\n", pid, strerror(errno));
+	} else {
+		ast_debug(1, "Sent TERM to pid %d%s\n", pid,
+			kill_method == KILL_METHOD_PROCESS_GROUP ? " and all children" : " only");
+	}
+	usleep(delay);
+	if (killer(pid, SIGKILL, kill_method) < 0) {
+		if (errno == ESRCH) {
+			return;
+		}
+		ast_log(LOG_WARNING, "Unable to kill MOH process '%d'?!!: %s\n", pid, strerror(errno));
+	} else {
+		ast_debug(1, "Sent KILL to pid %d%s\n", pid,
+			kill_method == KILL_METHOD_PROCESS_GROUP ? " and all children" : " only");
+	}
+}
+
 static void *monmp3thread(void *data)
 {
 #define	MOH_MS_INTERVAL		100
@@ -753,28 +807,7 @@
 				class->srcfd = -1;
 				pthread_testcancel();
 				if (class->pid > 1) {
-					do {
-						if (killpg(class->pid, SIGHUP) < 0) {
-							if (errno == ESRCH) {
-								break;
-							}
-							ast_log(LOG_WARNING, "Unable to send a SIGHUP to MOH process?!!: %s\n", strerror(errno));
-						}
-						usleep(100000);
-						if (killpg(class->pid, SIGTERM) < 0) {
-							if (errno == ESRCH) {
-								break;
-							}
-							ast_log(LOG_WARNING, "Unable to terminate MOH process?!!: %s\n", strerror(errno));
-						}
-						usleep(100000);
-						if (killpg(class->pid, SIGKILL) < 0) {
-							if (errno == ESRCH) {
-								break;
-							}
-							ast_log(LOG_WARNING, "Unable to kill MOH process?!!: %s\n", strerror(errno));
-						}
-					} while (0);
+					killpid(class->pid, class->kill_delay, class->kill_method);
 					class->pid = 0;
 				}
 			} else {
@@ -1328,6 +1361,7 @@
 		)) {
 		class->format = ao2_bump(ast_format_slin);
 		class->srcfd = -1;
+		class->kill_delay = 100000;
 	}
 
 	return class;
@@ -1600,44 +1634,22 @@
 
 	if (class->pid > 1) {
 		char buff[8192];
-		int bytes, tbytes = 0, stime = 0, pid = 0;
+		int bytes, tbytes = 0, stime = 0;
 
 		ast_debug(1, "killing %d!\n", class->pid);
 
 		stime = time(NULL) + 2;
-		pid = class->pid;
-		class->pid = 0;
-
-		/* Back when this was just mpg123, SIGKILL was fine.  Now we need
-		 * to give the process a reason and time enough to kill off its
-		 * children. */
-		do {
-			if (killpg(pid, SIGHUP) < 0) {
-				ast_log(LOG_WARNING, "Unable to send a SIGHUP to MOH process?!!: %s\n", strerror(errno));
-			}
-			usleep(100000);
-			if (killpg(pid, SIGTERM) < 0) {
-				if (errno == ESRCH) {
-					break;
-				}
-				ast_log(LOG_WARNING, "Unable to terminate MOH process?!!: %s\n", strerror(errno));
-			}
-			usleep(100000);
-			if (killpg(pid, SIGKILL) < 0) {
-				if (errno == ESRCH) {
-					break;
-				}
-				ast_log(LOG_WARNING, "Unable to kill MOH process?!!: %s\n", strerror(errno));
-			}
-		} while (0);
+		killpid(class->pid, class->kill_delay, class->kill_method);
 
 		while ((ast_wait_for_input(class->srcfd, 100) > 0) && 
 				(bytes = read(class->srcfd, buff, 8192)) && time(NULL) < stime) {
 			tbytes = tbytes + bytes;
 		}
 
-		ast_debug(1, "mpg123 pid %d and child died after %d bytes read\n", pid, tbytes);
+		ast_debug(1, "mpg123 pid %d and child died after %d bytes read\n",
+			class->pid, tbytes);
 
+		class->pid = 0;
 		close(class->srcfd);
 		class->srcfd = -1;
 	}
@@ -1764,6 +1776,22 @@
 				if (!class->format) {
 					ast_log(LOG_WARNING, "Unknown format '%s' -- defaulting to SLIN\n", var->value);
 					class->format = ao2_bump(ast_format_slin);
+				}
+			} else if (!strcasecmp(var->name, "kill_escalation_delay")) {
+				if (sscanf(var->value, "%zu", &class->kill_delay) == 1) {
+					class->kill_delay *= 1000;
+				} else {
+					ast_log(LOG_WARNING, "kill_escalation_delay '%s' is invalid.  Setting to 100ms\n", var->value);
+					class->kill_delay = 100000;
+				}
+			} else if (!strcasecmp(var->name, "kill_method")) {
+				if (!strcasecmp(var->value, "process")) {
+					class->kill_method = KILL_METHOD_PROCESS;
+				} else if (!strcasecmp(var->value, "process_group")){
+					class->kill_method = KILL_METHOD_PROCESS_GROUP;
+				} else {
+					ast_log(LOG_WARNING, "kill_method '%s' is invalid.  Setting to 'process_group'\n", var->value);
+					class->kill_method = KILL_METHOD_PROCESS_GROUP;
 				}
 			}
 		}
@@ -1899,6 +1927,9 @@
 		ast_cli(a->fd, "\tDirectory: %s\n", S_OR(class->dir, "<none>"));
 		if (ast_test_flag(class, MOH_CUSTOM)) {
 			ast_cli(a->fd, "\tApplication: %s\n", S_OR(class->args, "<none>"));
+			ast_cli(a->fd, "\tKill Escalation Delay: %zu ms\n", class->kill_delay / 1000);
+			ast_cli(a->fd, "\tKill Method: %s\n",
+				class->kill_method == KILL_METHOD_PROCESS ? "process" : "process_group");
 		}
 		if (strcasecmp(class->mode, "files")) {
 			ast_cli(a->fd, "\tFormat: %s\n", ast_format_get_name(class->format));

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

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-MessageType: merged
Gerrit-Change-Id: Iff70a1a9405685a9021a68416830c0db5158603b
Gerrit-Change-Number: 5984
Gerrit-PatchSet: 6
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Benjamin Keith Ford <bford at digium.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Matthew Fredrickson <creslin at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20170712/7d716b4d/attachment.html>


More information about the asterisk-code-review mailing list