[svn-commits] tilghman: trunk r232660 - in /trunk: include/asterisk/ res/
SVN commits to the Digium repositories
svn-commits at lists.digium.com
Wed Dec 2 18:09:03 CST 2009
Author: tilghman
Date: Wed Dec 2 18:08:55 2009
New Revision: 232660
URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=232660
Log:
Fix multiple issues with musiconhold, which led to classes not getting destroyed properly.
* Classes are now tracked past removal from the core container, and module
removal is actively prevented until all references are freed.
* A hanging reference stored in the channel has been removed. This could have
caused a mismatch and the music state not properly cleared, if two or more
reloads occurred between MOH being stopped and MOH being restarted.
* In certain circumstances, duplicate classes were possible.
* A race existed at reload time between a process being killed and the thread
responsible for reading from the related pipe respawning that process.
* Several reference counts have also been corrected. At least one could have
caused deleted classes to stick around forever, consuming resources. This
originally manifested as MOH external processes that were not killed at
reload time.
(closes issue #16279, closes issue #16207)
Reported by: parisioa, dcabot
Patches:
20091202__issue16279__2.diff.txt uploaded by tilghman (license 14)
Tested by: parisioa, tilghman
Modified:
trunk/include/asterisk/astobj2.h
trunk/res/res_musiconhold.c
Modified: trunk/include/asterisk/astobj2.h
URL: http://svnview.digium.com/svn/asterisk/trunk/include/asterisk/astobj2.h?view=diff&rev=232660&r1=232659&r2=232660
==============================================================================
--- trunk/include/asterisk/astobj2.h (original)
+++ trunk/include/asterisk/astobj2.h Wed Dec 2 18:08:55 2009
@@ -207,7 +207,7 @@
#ifdef REF_DEBUG
#define dialog_ref(arg1,arg2) dialog_ref_debug((arg1),(arg2), __FILE__, __LINE__, __PRETTY_FUNCTION__)
#define dialog_unref(arg1,arg2) dialog_unref_debug((arg1),(arg2), __FILE__, __LINE__, __PRETTY_FUNCTION__)
-static struct sip_pvt *dialog_ref_debug(struct sip_pvt *p, char *tag, char *file, int line, const char *func)
+static struct sip_pvt *dialog_ref_debug(struct sip_pvt *p, char *tag, const char *file, int line, const char *func)
{
if (p)
ao2_ref_debug(p, 1, tag, file, line, func);
@@ -216,7 +216,7 @@
return p;
}
-static struct sip_pvt *dialog_unref_debug(struct sip_pvt *p, char *tag, char *file, int line, const char *func)
+static struct sip_pvt *dialog_unref_debug(struct sip_pvt *p, char *tag, const char *file, int line, const char *func)
{
if (p)
ao2_ref_debug(p, -1, tag, file, line, func);
Modified: trunk/res/res_musiconhold.c
URL: http://svnview.digium.com/svn/asterisk/trunk/res/res_musiconhold.c?view=diff&rev=232660&r1=232659&r2=232660
==============================================================================
--- trunk/res/res_musiconhold.c (original)
+++ trunk/res/res_musiconhold.c Wed Dec 2 18:08:55 2009
@@ -47,6 +47,8 @@
#include <dahdi/user.h>
#endif
+#define REF_DEBUG
+
#include "asterisk/lock.h"
#include "asterisk/file.h"
#include "asterisk/channel.h"
@@ -66,6 +68,8 @@
#include "asterisk/astobj2.h"
#define INITIAL_NUM_FILES 8
+#define HANDLE_REF 1
+#define DONT_UNREF 0
/*** DOCUMENTATION
<application name="MusicOnHold" language="en_US">
@@ -148,11 +152,13 @@
struct moh_files_state {
struct mohclass *class;
+ char name[MAX_MUSICCLASS];
format_t origwfmt;
int samples;
int sample_queue;
int pos;
int save_pos;
+ int save_total;
char *save_pos_filename;
};
@@ -163,6 +169,9 @@
#define MOH_SORTALPHA (1 << 4)
#define MOH_CACHERTCLASSES (1 << 5) /*!< Should we use a separate instance of MOH for each user or not */
+
+/* Custom astobj2 flag */
+#define MOH_NOTDELETED (1 << 30) /*!< Find only records that aren't deleted? */
static struct ast_flags global_flags[1] = {{0}}; /*!< global MOH_ flags */
@@ -205,6 +214,8 @@
};
static struct ao2_container *mohclasses;
+static struct ao2_container *deleted_classes;
+static pthread_t deleted_thread;
#define LOCAL_MPG_123 "/usr/local/bin/mpg123"
#define MPG_123 "/usr/bin/mpg123"
@@ -240,7 +251,7 @@
state->save_pos = state->pos;
- mohclass_unref(state->class, "Unreffing channel's music class upon deactivation of generator");
+ state->class = mohclass_unref(state->class, "Unreffing channel's music class upon deactivation of generator");
}
static int ast_moh_files_next(struct ast_channel *chan)
@@ -349,7 +360,12 @@
return NULL;
}
- if (state->class != class) {
+ /* LOGIC: Comparing an unrefcounted pointer is a really bad idea, because
+ * malloc may allocate a different class to the same memory block. This
+ * might only happen when two reloads are generated in a short period of
+ * time, but it's still important to protect against.
+ * PROG: Compare the quick operation first, to save CPU. */
+ if (state->save_total != class->total_files || strcmp(state->name, class->name) != 0) {
memset(state, 0, sizeof(*state));
if (ast_test_flag(class, MOH_RANDOMIZE) && class->total_files) {
state->pos = ast_random() % class->total_files;
@@ -358,6 +374,9 @@
state->class = mohclass_ref(class, "Reffing music class for channel");
state->origwfmt = chan->writeformat;
+ /* For comparison on restart of MOH (see above) */
+ ast_copy_string(state->name, class->name, sizeof(state->name));
+ state->save_total = class->total_files;
ast_verb(3, "Started music on hold, class '%s', on %s\n", class->name, chan->name);
@@ -539,6 +558,43 @@
close(fds[1]);
}
return fds[0];
+}
+
+static void *deleted_monitor(void *data)
+{
+ struct ao2_iterator iter;
+ struct mohclass *class;
+ struct ast_module *mod = NULL;
+
+ for (;;) {
+ pthread_testcancel();
+ if (ao2_container_count(deleted_classes) == 0) {
+ if (mod) {
+ ast_module_unref(mod);
+ mod = NULL;
+ }
+ poll(NULL, 0, -1);
+ } else {
+ /* While deleted classes are still in use, prohibit unloading */
+ mod = ast_module_ref(ast_module_info->self);
+ }
+ pthread_testcancel();
+ iter = ao2_iterator_init(deleted_classes, 0);
+ while ((class = ao2_iterator_next(&iter))) {
+ if (ao2_ref(class, 0) == 2) {
+ ao2_unlink(deleted_classes, class);
+ }
+ ao2_ref(class, -1);
+ }
+ ao2_iterator_destroy(&iter);
+ if (ao2_container_count(deleted_classes) == 0 && mod) {
+ ast_module_unref(mod);
+ mod = NULL;
+ }
+ pthread_testcancel();
+ sleep(60);
+ }
+ return NULL;
}
static void *monmp3thread(void *data)
@@ -601,11 +657,25 @@
class->srcfd = -1;
pthread_testcancel();
if (class->pid > 1) {
- killpg(class->pid, SIGHUP);
- usleep(100000);
- killpg(class->pid, SIGTERM);
- usleep(100000);
- killpg(class->pid, SIGKILL);
+ do {
+ if (killpg(class->pid, SIGHUP) < 0) {
+ 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);
class->pid = 0;
}
} else {
@@ -734,7 +804,9 @@
return 0;
}
-static struct mohclass *get_mohbyname(const char *name, int warn)
+#define get_mohbyname(a,b,c) _get_mohbyname(a,b,c,__FILE__,__LINE__,__PRETTY_FUNCTION__)
+
+static struct mohclass *_get_mohbyname(const char *name, int warn, int flags, const char *file, int lineno, const char *funcname)
{
struct mohclass *moh = NULL;
struct mohclass tmp_class = {
@@ -743,7 +815,11 @@
ast_copy_string(tmp_class.name, name, sizeof(tmp_class.name));
- moh = ao2_t_find(mohclasses, &tmp_class, 0, "Finding by name");
+#ifdef REF_DEBUG
+ moh = __ao2_find_debug(mohclasses, &tmp_class, flags, "get_mohbyname", (char *) file, lineno, funcname);
+#else
+ moh = __ao2_find(mohclasses, &tmp_class, flags);
+#endif
if (!moh && warn) {
ast_log(LOG_DEBUG, "Music on Hold class '%s' not found in memory\n", name);
@@ -1084,22 +1160,23 @@
}
/*!
- * \note This function owns the reference it gets to moh
+ * \note This function owns the reference it gets to moh if unref is true
*/
-static int moh_register(struct mohclass *moh, int reload, int unref)
+#define moh_register(a,b,c) _moh_register(a,b,c,__FILE__,__LINE__,__PRETTY_FUNCTION__)
+static int _moh_register(struct mohclass *moh, int reload, int unref, const char *file, int line, const char *funcname)
{
struct mohclass *mohclass = NULL;
- if ((mohclass = get_mohbyname(moh->name, 0)) && !moh_diff(mohclass, moh)) {
- if (!mohclass->delete) {
- ast_log(LOG_WARNING, "Music on Hold class '%s' already exists\n", moh->name);
- mohclass = mohclass_unref(mohclass, "unreffing mohclass we just found by name");
- if (unref) {
- moh = mohclass_unref(moh, "unreffing potential new moh class (it is a duplicate)");
- }
- return -1;
- }
- mohclass = mohclass_unref(mohclass, "Unreffing mohclass we just found by name");
+ if ((mohclass = _get_mohbyname(moh->name, 0, MOH_NOTDELETED, file, line, funcname)) && !moh_diff(mohclass, moh)) {
+ ast_log(LOG_WARNING, "Music on Hold class '%s' already exists\n", moh->name);
+ mohclass = mohclass_unref(mohclass, "unreffing mohclass we just found by name");
+ if (unref) {
+ moh = mohclass_unref(moh, "unreffing potential new moh class (it is a duplicate)");
+ }
+ return -1;
+ } else if (mohclass) {
+ /* Found a class, but it's different from the one being registered */
+ mohclass = mohclass_unref(mohclass, "unreffing mohclass we just found by name");
}
time(&moh->start);
@@ -1137,6 +1214,9 @@
struct moh_files_state *state = chan->music_state;
if (state) {
+ if (state->class) {
+ state->class = mohclass_unref(state->class, "Channel MOH state destruction");
+ }
ast_free(chan->music_state);
chan->music_state = NULL;
}
@@ -1144,11 +1224,21 @@
static void moh_class_destructor(void *obj);
-static struct mohclass *moh_class_malloc(void)
+#define moh_class_malloc() _moh_class_malloc(__FILE__,__LINE__,__PRETTY_FUNCTION__)
+
+static struct mohclass *_moh_class_malloc(const char *file, int line, const char *funcname)
{
struct mohclass *class;
- if ((class = ao2_t_alloc(sizeof(*class), moh_class_destructor, "Allocating new moh class"))) {
+ if ((class =
+#ifdef REF_DEBUG
+ __ao2_alloc_debug(sizeof(*class), moh_class_destructor, "Allocating new moh class", file, line, funcname, 1)
+#elif defined(__AST_DEBUG_MALLOC)
+ __ao2_alloc_debug(sizeof(*class), moh_class_destructor, "Allocating new moh class", file, line, funcname, 0)
+#else
+ ao2_alloc(sizeof(*class), moh_class_destructor)
+#endif
+ )) {
class->format = AST_FORMAT_SLINEAR;
}
@@ -1175,26 +1265,26 @@
* 4) The default class.
*/
if (!ast_strlen_zero(chan->musicclass)) {
- mohclass = get_mohbyname(chan->musicclass, 1);
+ mohclass = get_mohbyname(chan->musicclass, 1, 0);
if (!mohclass && realtime_possible) {
var = ast_load_realtime("musiconhold", "name", chan->musicclass, SENTINEL);
}
}
if (!mohclass && !var && !ast_strlen_zero(mclass)) {
- mohclass = get_mohbyname(mclass, 1);
+ mohclass = get_mohbyname(mclass, 1, 0);
if (!mohclass && realtime_possible) {
var = ast_load_realtime("musiconhold", "name", mclass, SENTINEL);
}
}
if (!mohclass && !var && !ast_strlen_zero(interpclass)) {
- mohclass = get_mohbyname(interpclass, 1);
+ mohclass = get_mohbyname(interpclass, 1, 0);
if (!mohclass && realtime_possible) {
var = ast_load_realtime("musiconhold", "name", interpclass, SENTINEL);
}
}
if (!mohclass && !var) {
- mohclass = get_mohbyname("default", 1);
+ mohclass = get_mohbyname("default", 1, 0);
if (!mohclass && realtime_possible) {
var = ast_load_realtime("musiconhold", "name", "default", SENTINEL);
}
@@ -1272,7 +1362,7 @@
* has a pointer to a freed mohclass, so any operations involving the mohclass container would result in reading
* invalid memory.
*/
- moh_register(mohclass, 0, 0);
+ moh_register(mohclass, 0, DONT_UNREF);
} else {
/* We don't register RT moh class, so let's init it manualy */
@@ -1389,8 +1479,19 @@
{
struct mohclass *class = obj;
struct mohdata *member;
+ pthread_t tid = 0;
ast_debug(1, "Destroying MOH class '%s'\n", class->name);
+
+ /* Kill the thread first, so it cannot restart the child process while the
+ * class is being destroyed */
+ if (class->thread != AST_PTHREADT_NULL && class->thread != 0) {
+ tid = class->thread;
+ class->thread = AST_PTHREADT_NULL;
+ pthread_cancel(tid);
+ /* We'll collect the exit status later, after we ensure all the readers
+ * are dead. */
+ }
if (class->pid > 1) {
char buff[8192];
@@ -1405,11 +1506,25 @@
/* 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. */
- killpg(pid, SIGHUP);
- usleep(100000);
- killpg(pid, SIGTERM);
- usleep(100000);
- killpg(pid, SIGKILL);
+ 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);
while ((ast_wait_for_input(class->srcfd, 100) > 0) &&
(bytes = read(class->srcfd, buff, 8192)) && time(NULL) < stime) {
@@ -1423,12 +1538,6 @@
while ((member = AST_LIST_REMOVE_HEAD(&class->members, list))) {
free(member);
- }
-
- if (class->thread) {
- pthread_cancel(class->thread);
- pthread_join(class->thread, NULL);
- class->thread = AST_PTHREADT_NULL;
}
if (class->filearray) {
@@ -1439,6 +1548,11 @@
free(class->filearray);
class->filearray = NULL;
}
+
+ /* Finally, collect the exit status of the monitor thread */
+ if (tid > 0) {
+ pthread_join(tid, NULL);
+ }
}
static int moh_class_mark(void *obj, void *arg, int flags)
@@ -1454,7 +1568,12 @@
{
struct mohclass *class = obj;
- return class->delete ? CMP_MATCH : 0;
+ if (class->delete) {
+ ao2_link(deleted_classes, obj);
+ pthread_kill(deleted_thread, SIGURG);
+ return CMP_MATCH;
+ }
+ return 0;
}
static int load_moh_classes(int reload)
@@ -1546,7 +1665,7 @@
}
/* Don't leak a class when it's already registered */
- if (!moh_register(class, reload, 1)) {
+ if (!moh_register(class, reload, HANDLE_REF)) {
numclasses++;
}
}
@@ -1657,6 +1776,20 @@
}
}
ao2_iterator_destroy(&i);
+ i = ao2_iterator_init(deleted_classes, 0);
+ for (; (class = ao2_t_iterator_next(&i, "Show deleted classes iterator")); mohclass_unref(class, "Unref iterator in moh show classes")) {
+ ast_cli(a->fd, "(Deleted) Class: %s (%d)\n", class->name, ao2_ref(class, 0) - 2);
+ ast_cli(a->fd, "\tMode: %s\n", S_OR(class->mode, "<none>"));
+ ast_cli(a->fd, "\tDirectory: %s\n", S_OR(class->dir, "<none>"));
+ ast_cli(a->fd, "\tRealtime: %s\n", class->realtime ? "yes" : "no");
+ if (ast_test_flag(class, MOH_CUSTOM)) {
+ ast_cli(a->fd, "\tApplication: %s\n", S_OR(class->args, "<none>"));
+ }
+ if (strcasecmp(class->mode, "files")) {
+ ast_cli(a->fd, "\tFormat: %s\n", ast_getformatname(class->format));
+ }
+ }
+ ao2_iterator_destroy(&i);
return CLI_SUCCESS;
}
@@ -1678,7 +1811,9 @@
{
struct mohclass *class = obj, *class2 = arg;
- return strcasecmp(class->name, class2->name) ? 0 : CMP_MATCH | CMP_STOP;
+ return strcasecmp(class->name, class2->name) ? 0 :
+ (flags & MOH_NOTDELETED) && (class->delete || class2->delete) ? 0 :
+ CMP_MATCH | CMP_STOP;
}
static int load_module(void)
@@ -1686,6 +1821,14 @@
int res;
if (!(mohclasses = ao2_t_container_alloc(53, moh_class_hash, moh_class_cmp, "Moh class container"))) {
+ return AST_MODULE_LOAD_DECLINE;
+ }
+
+ if (!(deleted_classes = ao2_t_container_alloc(53, moh_class_hash, moh_class_cmp, "Moh deleted class container"))) {
+ return AST_MODULE_LOAD_DECLINE;
+ }
+
+ if (ast_pthread_create_background(&deleted_thread, NULL, deleted_monitor, NULL)) {
return AST_MODULE_LOAD_DECLINE;
}
@@ -1757,6 +1900,10 @@
ast_cli_unregister_multiple(cli_moh, ARRAY_LEN(cli_moh));
ast_unregister_atexit(ast_moh_destroy);
+ pthread_cancel(deleted_thread);
+ pthread_kill(deleted_thread, SIGURG);
+ pthread_join(deleted_thread, NULL);
+
return res;
}
More information about the svn-commits
mailing list