Fix multiple issues with musiconhold, which led to classes not getting destroyed...
authorTilghman Lesher <tilghman@meg.abyt.es>
Thu, 3 Dec 2009 00:08:55 +0000 (00:08 +0000)
committerTilghman Lesher <tilghman@meg.abyt.es>
Thu, 3 Dec 2009 00:08:55 +0000 (00:08 +0000)
 * 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

git-svn-id: https://origsvn.digium.com/svn/asterisk/trunk@232660 65c4cc65-6c06-0410-ace0-fbb531ad65f3

include/asterisk/astobj2.h
res/res_musiconhold.c

index 6caa4a7..689b639 100644 (file)
@@ -207,7 +207,7 @@ were called to appear in /tmp/refs, you can do this sort of thing:
 #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 @@ static struct sip_pvt *dialog_ref_debug(struct sip_pvt *p, char *tag, char *file
        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);
index 410a232..288941e 100644 (file)
@@ -47,6 +47,8 @@ ASTERISK_FILE_VERSION(__FILE__, "$Revision$")
 #include <dahdi/user.h>
 #endif
 
+#define REF_DEBUG
+
 #include "asterisk/lock.h"
 #include "asterisk/file.h"
 #include "asterisk/channel.h"
@@ -66,6 +68,8 @@ ASTERISK_FILE_VERSION(__FILE__, "$Revision$")
 #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 @@ static int respawn_time = 20;
 
 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;
 };
 
@@ -164,6 +170,9 @@ struct moh_files_state {
 
 #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 */
 
 struct mohclass {
@@ -205,6 +214,8 @@ struct mohdata {
 };
 
 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 @@ static void moh_files_release(struct ast_channel *chan, void *data)
 
        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 @@ static void *moh_files_alloc(struct ast_channel *chan, void *params)
                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 @@ static void *moh_files_alloc(struct ast_channel *chan, void *params)
 
        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);
        
@@ -541,6 +560,43 @@ static int spawn_mp3(struct mohclass *class)
        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)
 {
 #define        MOH_MS_INTERVAL         100
@@ -601,11 +657,25 @@ static void *monmp3thread(void *data)
                                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 @@ static int stop_moh_exec(struct ast_channel *chan, const char *data)
        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 @@ static struct mohclass *get_mohbyname(const char *name, int warn)
 
        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 @@ static int init_app_class(struct mohclass *class)
 }
 
 /*!
- * \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;
+       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)");
                }
-               mohclass = mohclass_unref(mohclass, "Unreffing mohclass we just found by name");
+               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 @@ static void local_ast_moh_cleanup(struct ast_channel *chan)
        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 local_ast_moh_cleanup(struct ast_channel *chan)
 
 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 @@ static int local_ast_moh_start(struct ast_channel *chan, const char *mclass, con
         * 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 @@ static int local_ast_moh_start(struct ast_channel *chan, const char *mclass, con
                                 * 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,9 +1479,20 @@ static void moh_class_destructor(void *obj)
 {
        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];
                int bytes, tbytes = 0, stime = 0, pid = 0;
@@ -1405,11 +1506,25 @@ static void moh_class_destructor(void *obj)
                /* 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) {
@@ -1425,12 +1540,6 @@ static void moh_class_destructor(void *obj)
                free(member);
        }
 
-       if (class->thread) {
-               pthread_cancel(class->thread);
-               pthread_join(class->thread, NULL);
-               class->thread = AST_PTHREADT_NULL;
-       }
-
        if (class->filearray) {
                int i;
                for (i = 0; i < class->total_files; i++) {
@@ -1439,6 +1548,11 @@ static void moh_class_destructor(void *obj)
                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 @@ static int moh_classes_delete_marked(void *obj, void *arg, int flags)
 {
        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 @@ static int load_moh_classes(int reload)
                }
 
                /* 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 @@ static char *handle_cli_moh_show_classes(struct ast_cli_entry *e, int cmd, struc
                }
        }
        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 @@ static int moh_class_cmp(void *obj, void *arg, int flags)
 {
        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)
@@ -1689,6 +1824,14 @@ static int load_module(void)
                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;
+       }
+
        if (!load_moh_classes(0)) {     /* No music classes configured, so skip it */
                ast_log(LOG_WARNING, "No music on hold classes configured, "
                                "disabling music on hold.\n");
@@ -1757,6 +1900,10 @@ static int unload_module(void)
        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;
 }