Skip to content

replaygain: Fix error handling for parallel runs#4506

Merged
sampsyo merged 3 commits intomasterfrom
replaygain-fix-exc
Oct 1, 2022
Merged

replaygain: Fix error handling for parallel runs#4506
sampsyo merged 3 commits intomasterfrom
replaygain-fix-exc

Conversation

@sampsyo
Copy link
Copy Markdown
Member

@sampsyo sampsyo commented Oct 1, 2022

This is a second fix to errors that have been cropping up in CI lately. Put this one into the bucket labeled "I don't know why this ever worked," but it's causing problems only on the Windows CI builds.

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.

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.
@sampsyo
Copy link
Copy Markdown
Member Author

sampsyo commented Oct 1, 2022

This fixes the obvious crash in the error handling. However, there is still a problem with invoking ffmpeg on Unicode filenames on Windows. I'll work on that in another PR—but if that effort fails (it might!), then we can always go back to giving up entirely in situations like this. (Unicode stuff on Windows has always been a mess.)

@sampsyo sampsyo merged commit c8ab0cf into master Oct 1, 2022
@sampsyo sampsyo deleted the replaygain-fix-exc branch October 1, 2022 23:35
@wisp3rwind
Copy link
Copy Markdown
Member

This fixes the obvious crash in the error handling. However, there is still a problem with invoking ffmpeg on Unicode filenames on Windows. I'll work on that in another PR—but if that effort fails (it might!), then we can always go back to giving up entirely in situations like this. (Unicode stuff on Windows has always been a mess.)

👍

A few days ago, I've also realized this when testing #4374, which was also plagued by mysterious CI failures that I couldn't trace back to any of the changes there. However, your fix is much cleaner than the quick workaround I came up with!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants