From 9803939a1c3e0ca1811cce2dd763fe7dba52ef39 Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Sat, 1 Oct 2022 15:47:41 -0700 Subject: [PATCH 1/3] replaygain: Fix error handling for parallel runs The parallelism strategy in #3478, in retrospect, used a pretty funky way to deal with exceptions in the asynchronous work---since `apply_async` has an `error_callback` parameter that's meant for exactly this. The problem is that the wrapped function would correctly log the exception *and then return `None`*, confusing any downstream code. Instead of just adding `None`-awareness to the callback, let's just avoid running the callback altogether in the case of an error. --- beetsplug/replaygain.py | 27 ++++++++++----------------- 1 file changed, 10 insertions(+), 17 deletions(-) diff --git a/beetsplug/replaygain.py b/beetsplug/replaygain.py index c228f74b3f..63d927f619 100644 --- a/beetsplug/replaygain.py +++ b/beetsplug/replaygain.py @@ -1085,7 +1085,7 @@ def run(self): try: exc = self._queue.get_nowait() self._callback() - raise exc[1].with_traceback(exc[2]) + raise exc except queue.Empty: # No exceptions yet, loop back to check # whether `_stopevent` is set @@ -1338,23 +1338,16 @@ def open_pool(self, threads): def _apply(self, func, args, kwds, callback): if self._has_pool(): - def catch_exc(func, exc_queue, log): - """Wrapper to catch raised exceptions in threads + def handle_exc(exc): + """Handle exceptions in the async work. """ - def wfunc(*args, **kwargs): - try: - return func(*args, **kwargs) - except ReplayGainError as e: - log.info(e.args[0]) # log non-fatal exceptions - except Exception: - exc_queue.put(sys.exc_info()) - return wfunc - - # Wrap function and callback to catch exceptions - func = catch_exc(func, self.exc_queue, self._log) - callback = catch_exc(callback, self.exc_queue, self._log) - - self.pool.apply_async(func, args, kwds, callback) + if isinstance(exc, ReplayGainError): + self._log.info(exc.args[0]) # Log non-fatal exceptions. + else: + self.exc_queue.put(exc) + + self.pool.apply_async(func, args, kwds, callback, + error_callback=handle_exc) else: callback(func(*args, **kwds)) From eaabf291f7edf3defbf469e251789ca2b0842d7c Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Sat, 1 Oct 2022 15:51:44 -0700 Subject: [PATCH 2/3] Changelog for #4506 --- docs/changelog.rst | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/changelog.rst b/docs/changelog.rst index 11f04dee44..9df77d5e7f 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -48,6 +48,9 @@ New features: Bug fixes: +* :doc:`/plugins/replaygain`: Avoid a crash when errors occur in the analysis + backend. + :bug:`4506` * We now respect the Spotify API's rate limiting, which avoids crashing when the API reports code 429 (too many requests). :bug:`4370` * Fix implicit paths OR queries (e.g. ``beet list /path/ , /other-path/``) From 675dd7b9a9f6cf7023225fd578f4fdde3045cf2b Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Sat, 1 Oct 2022 16:06:40 -0700 Subject: [PATCH 3/3] Add logging for command output on errors --- beetsplug/replaygain.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/beetsplug/replaygain.py b/beetsplug/replaygain.py index 63d927f619..ab3db45759 100644 --- a/beetsplug/replaygain.py +++ b/beetsplug/replaygain.py @@ -49,13 +49,14 @@ class FatalGstreamerPluginReplayGainError(FatalReplayGainError): loading the required plugins.""" -def call(args, **kwargs): +def call(args, log, **kwargs): """Execute the command and return its output or raise a ReplayGainError on failure. """ try: return command_output(args, **kwargs) except subprocess.CalledProcessError as e: + log.debug(e.output.decode('utf8', 'ignore')) raise ReplayGainError( "{} exited with status {}".format(args[0], e.returncode) ) @@ -261,7 +262,7 @@ def __init__(self, config, log): # check that ffmpeg is installed try: - ffmpeg_version_out = call([self._ffmpeg_path, "-version"]) + ffmpeg_version_out = call([self._ffmpeg_path, "-version"], log) except OSError: raise FatalReplayGainError( f"could not find ffmpeg at {self._ffmpeg_path}" @@ -394,7 +395,7 @@ def _analyse_item(self, item, target_level, peak_method, self._log.debug( 'executing {0}', ' '.join(map(displayable_path, cmd)) ) - output = call(cmd).stderr.splitlines() + output = call(cmd, self._log).stderr.splitlines() # parse output @@ -525,7 +526,7 @@ def __init__(self, config, log): # Check whether the program is in $PATH. for cmd in ('mp3gain', 'aacgain'): try: - call([cmd, '-v']) + call([cmd, '-v'], self._log) self.command = cmd except OSError: pass @@ -605,7 +606,7 @@ def compute_gain(self, items, target_level, is_album): self._log.debug('analyzing {0} files', len(items)) self._log.debug("executing {0}", " ".join(map(displayable_path, cmd))) - output = call(cmd).stdout + output = call(cmd, self._log).stdout self._log.debug('analysis finished') return self.parse_tool_output(output, len(items) + (1 if is_album else 0))