Fix issue 61: explicitly close stderr stream when read_frames exits

imageio / imageio-ffmpeg

FFMPEG wrapper for Python

BSD 2-Clause "Simplified" License

221 stars 50 forks source link

Fix issue 61: explicitly close stderr stream when read_frames exits #62

Closed dennisvang closed 2 years ago

dennisvang commented 2 years ago

This should fix issue #61.

My initial inclination was to call p.stderr.close() directly after closing stdout and stdin, here. However, that caused Windows fatal exception: access violation in test_io.py.

almarklein commented 2 years ago

Thanks! Also nice to have the extra test to avoid regressions.

Is there a particular reason for adding this close() at the end instead of put it where the other pipes are closed?

FirefoxMetzger commented 2 years ago

I will leave this review to you @almarklein . I still haven't found any time to look through the imageio-ffmpeg codebase, so I don't feel confident doing reviews for this project yet.

(It will have to wait until after the imageio v3 docs are written and I've implemented the metadata API.)

dennisvang commented 2 years ago

Is there a particular reason for adding this close() at the end instead of put it where the other pipes are closed?

@almarklein Yes there is: I also tried closing stderr right after stdin (here), but that caused a Windows fatal exception: access violation during tests in test_io.py. Not quite sure why...

almarklein commented 2 years ago

I played around a bit:

Commented the p.stderr.close() and then all tests pass except the newly added.
Moved the close() upwards where the other pipes are closed, even surrounding it with a try-except: segfaults on most builds.
Tweaked CI to complete all builds also if one fails, and put things back the way you wrote them. Most builds segfault.

It looks like there is no safe way to close the err pipe. I would also think that we don't need to, because the subprocess it is attached to is closed one way or another. Maybe we need to suppress the warning instead?

dennisvang commented 2 years ago

@almarklein Stopping the log_catcher before closing stderr prevents the access violation I was seeing on windows. I guess that would make sense, as the log_catcher was probably trying to read from the closed stderr pipe.

dennisvang commented 2 years ago

@almarklein I'm just wondering: should the log_catcher be stopped at the start of the finally block, i.e. before the if p.poll() is None ?

almarklein commented 2 years ago

Oh nice catch! And it seems to work :)

Yes, I think it makes sense to stop the log_catcher at the start of the finally block, since there is no need for it any more.

almarklein commented 2 years ago

Awesome, thanks for this!