diff --git a/bragi.py b/bragi.py index 9072561..b5008d4 100755 --- a/bragi.py +++ b/bragi.py @@ -547,8 +547,7 @@ class MumbleBot: elif self.on_interrupting or len(raw_music) < self.pcm_buffer_size: self.mumble.sound_output.add_sound( audioop.mul(self._fadeout(raw_music, self.stereo, fadein=False), 2, self.volume_helper.real_volume)) - self.thread.kill() - self.thread = None + self._cleanup_ffmpeg_process() time.sleep(0.1) self.on_interrupting = False else: @@ -557,7 +556,7 @@ class MumbleBot: time.sleep(0.1) if not self.is_pause and not raw_music: - self.thread = None + self._cleanup_ffmpeg_process() # bot is not paused, but ffmpeg thread has gone. # indicate that last song has finished, or the bot just resumed from pause, or something is wrong. if self.read_pcm_size < self.pcm_buffer_size \ @@ -703,12 +702,14 @@ class MumbleBot: def clear(self): # Kill the ffmpeg thread and empty the playlist self.interrupt() + self._cleanup_ffmpeg_process() # Ensure proper cleanup var.playlist.clear() self.wait_for_ready = False self.log.info("bot: music stopped. playlist trashed.") def stop(self): self.interrupt() + self._cleanup_ffmpeg_process() # Ensure proper cleanup self.is_pause = True if len(var.playlist) > 0: self.wait_for_ready = True @@ -716,6 +717,34 @@ class MumbleBot: self.wait_for_ready = False self.log.info("bot: music stopped.") + def _cleanup_ffmpeg_process(self): + """Properly cleanup FFmpeg process and associated resources""" + if self.thread: + try: + # Check if process is already terminated to avoid double cleanup + if self.thread.poll() is None: # Process is still running + self.thread.terminate() # Send SIGTERM first + try: + self.thread.wait(timeout=2) # Wait up to 2 seconds for graceful exit + except sp.TimeoutExpired: + self.log.warning("bot: FFmpeg process didn't terminate gracefully, killing forcefully") + self.thread.kill() # Force kill if it doesn't terminate + self.thread.wait() # Wait for process to be reaped + else: + # Process already terminated, just wait to reap it + self.thread.wait() + except Exception as e: + self.log.error(f"bot: Error cleaning up FFmpeg process: {e}") + finally: + # Close stderr pipe if it exists + if self.thread_stderr: + try: + self.thread_stderr.close() + self.thread_stderr = None + except Exception as e: + self.log.error(f"bot: Error closing stderr pipe: {e}") + self.thread = None + def interrupt(self): # Kill the ffmpeg thread if self.thread: @@ -728,6 +757,7 @@ class MumbleBot: def pause(self): # Kill the ffmpeg thread self.interrupt() + self._cleanup_ffmpeg_process() # Ensure proper cleanup self.is_pause = True self.song_start_at = -1 if len(var.playlist) > 0: diff --git a/database.py b/database.py index ab64932..5138920 100644 --- a/database.py +++ b/database.py @@ -208,10 +208,14 @@ class SettingsDatabase: def get(self, section, option, **kwargs): conn = sqlite3.connect(self.db_path) - cursor = conn.cursor() - result = cursor.execute("SELECT value FROM botamusique WHERE section=? AND option=?", - (section, option)).fetchall() - conn.close() + try: + cursor = conn.cursor() + result = cursor.execute("SELECT value FROM botamusique WHERE section=? AND option=?", + (section, option)).fetchall() + finally: + if 'cursor' in locals(): + cursor.close() + conn.close() if len(result) > 0: return result[0][0] @@ -232,18 +236,26 @@ class SettingsDatabase: def set(self, section, option, value): conn = sqlite3.connect(self.db_path) - cursor = conn.cursor() - cursor.execute("INSERT OR REPLACE INTO botamusique (section, option, value) " - "VALUES (?, ?, ?)", (section, option, value)) - conn.commit() - conn.close() + try: + cursor = conn.cursor() + cursor.execute("INSERT OR REPLACE INTO botamusique (section, option, value) " + "VALUES (?, ?, ?)", (section, option, value)) + conn.commit() + finally: + if 'cursor' in locals(): + cursor.close() + conn.close() def has_option(self, section, option): conn = sqlite3.connect(self.db_path) - cursor = conn.cursor() - result = cursor.execute("SELECT value FROM botamusique WHERE section=? AND option=?", - (section, option)).fetchall() - conn.close() + try: + cursor = conn.cursor() + result = cursor.execute("SELECT value FROM botamusique WHERE section=? AND option=?", + (section, option)).fetchall() + finally: + if 'cursor' in locals(): + cursor.close() + conn.close() if len(result) > 0: return True else: @@ -251,23 +263,35 @@ class SettingsDatabase: def remove_option(self, section, option): conn = sqlite3.connect(self.db_path) - cursor = conn.cursor() - cursor.execute("DELETE FROM botamusique WHERE section=? AND option=?", (section, option)) - conn.commit() - conn.close() + try: + cursor = conn.cursor() + cursor.execute("DELETE FROM botamusique WHERE section=? AND option=?", (section, option)) + conn.commit() + finally: + if 'cursor' in locals(): + cursor.close() + conn.close() def remove_section(self, section): conn = sqlite3.connect(self.db_path) - cursor = conn.cursor() - cursor.execute("DELETE FROM botamusique WHERE section=?", (section,)) - conn.commit() - conn.close() + try: + cursor = conn.cursor() + cursor.execute("DELETE FROM botamusique WHERE section=?", (section,)) + conn.commit() + finally: + if 'cursor' in locals(): + cursor.close() + conn.close() def items(self, section): conn = sqlite3.connect(self.db_path) - cursor = conn.cursor() - results = cursor.execute("SELECT option, value FROM botamusique WHERE section=?", (section,)).fetchall() - conn.close() + try: + cursor = conn.cursor() + results = cursor.execute("SELECT option, value FROM botamusique WHERE section=?", (section,)).fetchall() + finally: + if 'cursor' in locals(): + cursor.close() + conn.close() if len(results) > 0: return list(map(lambda v: (v[0], v[1]), results)) @@ -276,9 +300,13 @@ class SettingsDatabase: def drop_table(self): conn = sqlite3.connect(self.db_path) - cursor = conn.cursor() - cursor.execute("DROP TABLE botamusique") - conn.close() + try: + cursor = conn.cursor() + cursor.execute("DROP TABLE botamusique") + finally: + if 'cursor' in locals(): + cursor.close() + conn.close() class MusicDatabase: @@ -287,66 +315,79 @@ class MusicDatabase: def insert_music(self, music_dict, _conn=None): conn = sqlite3.connect(self.db_path) if _conn is None else _conn - cursor = conn.cursor() + cursor = None + try: + cursor = conn.cursor() - id = music_dict['id'] - title = music_dict['title'] - type = music_dict['type'] - path = music_dict['path'] if 'path' in music_dict else '' - keywords = music_dict['keywords'] + id = music_dict['id'] + title = music_dict['title'] + type = music_dict['type'] + path = music_dict['path'] if 'path' in music_dict else '' + keywords = music_dict['keywords'] - tags_list = list(dict.fromkeys(music_dict['tags'])) - tags = '' - if tags_list: - tags = ",".join(tags_list) + "," + tags_list = list(dict.fromkeys(music_dict['tags'])) + tags = '' + if tags_list: + tags = ",".join(tags_list) + "," - del music_dict['id'] - del music_dict['title'] - del music_dict['type'] - del music_dict['tags'] - if 'path' in music_dict: - del music_dict['path'] - del music_dict['keywords'] + del music_dict['id'] + del music_dict['title'] + del music_dict['type'] + del music_dict['tags'] + if 'path' in music_dict: + del music_dict['path'] + del music_dict['keywords'] - existed = cursor.execute("SELECT 1 FROM music WHERE id=?", (id,)).fetchall() - if len(existed) == 0: - cursor.execute( - "INSERT INTO music (id, type, title, metadata, tags, path, keywords) VALUES (?, ?, ?, ?, ?, ?, ?)", - (id, - type, - title, - json.dumps(music_dict), - tags, - path, - keywords)) - else: - cursor.execute("UPDATE music SET type=:type, title=:title, metadata=:metadata, tags=:tags, " - "path=:path, keywords=:keywords WHERE id=:id", - {'id': id, - 'type': type, - 'title': title, - 'metadata': json.dumps(music_dict), - 'tags': tags, - 'path': path, - 'keywords': keywords}) + existed = cursor.execute("SELECT 1 FROM music WHERE id=?", (id,)).fetchall() + if len(existed) == 0: + cursor.execute( + "INSERT INTO music (id, type, title, metadata, tags, path, keywords) VALUES (?, ?, ?, ?, ?, ?, ?)", + (id, + type, + title, + json.dumps(music_dict), + tags, + path, + keywords)) + else: + cursor.execute("UPDATE music SET type=:type, title=:title, metadata=:metadata, tags=:tags, " + "path=:path, keywords=:keywords WHERE id=:id", + {'id': id, + 'type': type, + 'title': title, + 'metadata': json.dumps(music_dict), + 'tags': tags, + 'path': path, + 'keywords': keywords}) + finally: + if cursor: + cursor.close() if not _conn: conn.commit() conn.close() def query_music_ids(self, condition: Condition): conn = sqlite3.connect(self.db_path) - cursor = conn.cursor() - results = cursor.execute("SELECT id FROM music WHERE id != 'info' AND %s" % - condition.sql(conn), condition.filler).fetchall() - conn.close() + try: + cursor = conn.cursor() + results = cursor.execute("SELECT id FROM music WHERE id != 'info' AND %s" % + condition.sql(conn), condition.filler).fetchall() + finally: + if 'cursor' in locals(): + cursor.close() + conn.close() return list(map(lambda i: i[0], results)) def query_all_paths(self): conn = sqlite3.connect(self.db_path) - cursor = conn.cursor() - results = cursor.execute("SELECT path FROM music WHERE id != 'info' AND type = 'file'").fetchall() - conn.close() + try: + cursor = conn.cursor() + results = cursor.execute("SELECT path FROM music WHERE id != 'info' AND type = 'file'").fetchall() + finally: + if 'cursor' in locals(): + cursor.close() + conn.close() paths = [] for result in results: if result and result[0]: @@ -356,25 +397,33 @@ class MusicDatabase: def query_all_tags(self): conn = sqlite3.connect(self.db_path) - cursor = conn.cursor() - results = cursor.execute("SELECT tags FROM music WHERE id != 'info'").fetchall() - tags = [] - for result in results: - for tag in result[0].strip(",").split(","): - if tag and tag not in tags: - tags.append(tag) - conn.close() + try: + cursor = conn.cursor() + results = cursor.execute("SELECT tags FROM music WHERE id != 'info'").fetchall() + tags = [] + for result in results: + for tag in result[0].strip(",").split(","): + if tag and tag not in tags: + tags.append(tag) + finally: + if 'cursor' in locals(): + cursor.close() + conn.close() return tags def query_music_count(self, condition: Condition): filler = condition.filler conn = sqlite3.connect(self.db_path) - condition_str = condition.sql(conn) - cursor = conn.cursor() - results = cursor.execute("SELECT COUNT(*) FROM music " - "WHERE id != 'info' AND %s" % condition_str, filler).fetchall() - conn.close() + try: + condition_str = condition.sql(conn) + cursor = conn.cursor() + results = cursor.execute("SELECT COUNT(*) FROM music " + "WHERE id != 'info' AND %s" % condition_str, filler).fetchall() + finally: + if 'cursor' in locals(): + cursor.close() + conn.close() return results[0][0] @@ -382,10 +431,15 @@ class MusicDatabase: filler = condition.filler conn = sqlite3.connect(self.db_path) if _conn is None else _conn - condition_str = condition.sql(conn) - cursor = conn.cursor() - results = cursor.execute("SELECT id, type, title, metadata, tags, path, keywords FROM music " - "WHERE id != 'info' AND %s" % condition_str, filler).fetchall() + cursor = None + try: + condition_str = condition.sql(conn) + cursor = conn.cursor() + results = cursor.execute("SELECT id, type, title, metadata, tags, path, keywords FROM music " + "WHERE id != 'info' AND %s" % condition_str, filler).fetchall() + finally: + if cursor: + cursor.close() if not _conn: conn.close() @@ -393,9 +447,14 @@ class MusicDatabase: def _query_music_by_plain_sql_cond(self, sql_cond, _conn=None): conn = sqlite3.connect(self.db_path) if _conn is None else _conn - cursor = conn.cursor() - results = cursor.execute("SELECT id, type, title, metadata, tags, path, keywords FROM music " - "WHERE id != 'info' AND %s" % sql_cond).fetchall() + cursor = None + try: + cursor = conn.cursor() + results = cursor.execute("SELECT id, type, title, metadata, tags, path, keywords FROM music " + "WHERE id != 'info' AND %s" % sql_cond).fetchall() + finally: + if cursor: + cursor.close() if not _conn: conn.close() diff --git a/pymumble_py3/callbacks.py b/pymumble_py3/callbacks.py index b46e883..895eef1 100644 --- a/pymumble_py3/callbacks.py +++ b/pymumble_py3/callbacks.py @@ -81,6 +81,7 @@ class CallBacks(dict): for func in self[callback]: if callback is PYMUMBLE_CLBK_TEXTMESSAGERECEIVED: thr = threading.Thread(target=func, args=pos_parameters) + thr.daemon = True # Fix critical memory leak - ensure thread cleanup thr.start() else: func(*pos_parameters) diff --git a/pymumble_py3/soundoutput.py b/pymumble_py3/soundoutput.py index 678a71d..ea750a2 100644 --- a/pymumble_py3/soundoutput.py +++ b/pymumble_py3/soundoutput.py @@ -161,14 +161,27 @@ class SoundOutput: samples = int(self.encoder_framesize * PYMUMBLE_SAMPLERATE * 2 * self.channels) # number of samples in an encoder frame self.lock.acquire() - if len(self.pcm) and len(self.pcm[-1]) < samples: - initial_offset = samples - len(self.pcm[-1]) - self.pcm[-1] += pcm[:initial_offset] - else: - initial_offset = 0 - for i in range(initial_offset, len(pcm), samples): - self.pcm.append(pcm[i:i + samples]) - self.lock.release() + try: + # Prevent unbounded buffer growth - limit to 10 seconds of audio + max_buffer_seconds = 10.0 + max_buffer_frames = int(max_buffer_seconds / self.encoder_framesize) + + # If buffer is too full, drop oldest frames to prevent memory leak + if len(self.pcm) > max_buffer_frames: + frames_to_drop = len(self.pcm) - max_buffer_frames + self.pcm = self.pcm[frames_to_drop:] + if hasattr(self, 'Log'): + self.Log.warning(f"Audio buffer overflow, dropped {frames_to_drop} frames") + + if len(self.pcm) and len(self.pcm[-1]) < samples: + initial_offset = samples - len(self.pcm[-1]) + self.pcm[-1] += pcm[:initial_offset] + else: + initial_offset = 0 + for i in range(initial_offset, len(pcm), samples): + self.pcm.append(pcm[i:i + samples]) + finally: + self.lock.release() def clear_buffer(self): self.lock.acquire() diff --git a/util.py b/util.py index cd4845a..1916ce0 100644 --- a/util.py +++ b/util.py @@ -312,15 +312,29 @@ def get_media_duration(path): command = ("ffprobe", "-v", "quiet", "-show_entries", "format=duration", "-of", "default=noprint_wrappers=1:nokey=1", path) process = sp.Popen(command, stdout=sp.PIPE, stderr=sp.PIPE) - stdout, stderr = process.communicate() - try: - if not stderr: - return float(stdout) - else: + stdout, stderr = process.communicate(timeout=10) # Add timeout to prevent hanging + + try: + if not stderr: + return float(stdout) + else: + return 0 + except ValueError: return 0 - except ValueError: + except sp.TimeoutExpired: + process.kill() + process.wait() return 0 + finally: + # Ensure process is properly cleaned up + if process.poll() is None: + process.terminate() + try: + process.wait(timeout=2) + except sp.TimeoutExpired: + process.kill() + process.wait() def parse_time(human): @@ -405,9 +419,9 @@ def get_snapshot_version(): ver = "unknown" if os.path.exists(os.path.join(root_dir, ".git")): try: - ret = subprocess.check_output(["git", "describe", "--tags"]).strip() + ret = subprocess.check_output(["git", "describe", "--tags"], timeout=5).strip() ver = ret.decode("utf-8") - except (FileNotFoundError, subprocess.CalledProcessError): + except (FileNotFoundError, subprocess.CalledProcessError, subprocess.TimeoutExpired): try: with open(os.path.join(root_dir, ".git/refs/heads/master")) as f: ver = "g" + f.read()[:7]