Close file handles in ffdec.py #68
Conversation
audioread/ffdec.py
Outdated
| @@ -264,12 +264,14 @@ def close(self): | |||
| # ffmpeg closes normally on its own, but never updates | |||
| # `returncode`. | |||
| self.proc.poll() | |||
| self.proc.stdout.close() | |||
| self.proc.stderr.close() | |||
There was a problem hiding this comment.
The comment above doesn't apply to these lines, so it would be nice to use a blank line above them—and to write a new comment explaining why these are here. (For posterity, for example, we might want to record why this goes above the if block that happens next.)
There was a problem hiding this comment.
Done -- left a brief note.
audioread/ffdec.py
Outdated
|
|
||
| # Kill the process if it is still running. | ||
| if self.proc.returncode is None: | ||
| self.proc.kill() | ||
| self.proc.wait() | ||
| self.devnull.close() | ||
| self.devnull.close() |
There was a problem hiding this comment.
Again, since this is separate from the above handling of the subprocess, it would be a good idea to put a blank line above this one. I'm not sure if an explanatory comment would be useful to say something about why this is necessary.
There was a problem hiding this comment.
I added a comment anyway -- feel free to remove it if you believe it is too verbose. :)
|
So we can refer back to it later, what sort of resource leak warnings were you getting? Are they Python messages? Or something from ffmpeg? |
|
The errors were Python errors, coming from an execution of a unit test with python3 -m unittest. I think it enables these sorts of warning if an FD is left open after a test finishes. I will copy/paste an error later tonight. |
Close file handles in ffdec.py
|
Awesome; thanks for clarifying! I merged the changes. ✨ |
|
This change is triggering a segfault in Beets, see #62 |
Hello!
Thanks for all the hard work putting this library together! It made quickly combining two audio files I had super easy.
This PR just closes the /dev/null, stdout, and stderr FDs when in FFmpeg mode to avoid resource leak warnings.
Let me know if there are any issues,
Ryan