Fix hang in read_frames fixes #17

[christopherhesse]

This is to fix #17, it's not clear to me why p.communicate() with a timeout still hangs, nor why p.send_signal(signal.SIGINT) doesn't cause a graceful exit. But since they don't seem to work, and this is reading data, not writing it, seems fine to simply terminate the subprocess.

Is there any downside to this @almarklein?

[almarklein]

mmm, this would be a bit rough. It's generally not nice to stop a process this way ...

Just to be sure, did you try something like this?

try:
    p.communicate(b"q", timeout=0.5)
except subprocess.TimeoutExpired:
    p.send_signal(signal.SIGINT)

[christopherhesse]

Yeah it just hung in communicate (which doesn't make sense to me as I mentioned in the original post). What's not nice about stopping a process this way? Is it going to leak file descriptors or something?

[christopherhesse]

Also in your tests on Linux, does sending SIGINT with the currently released version of imageio-ffmpeg even do anything? It didn't seem to work when I tried it.

[almarklein]

mmm, odd that the timeout does not work. What about sending q the easy way (and then waiting for it to end as we already did:

try:
    p.stdin.write("q")
    p.stind.close()
except xx:  # whatever we need to catch when we stdin is already closed
   pass

[almarklein]

What's not nice about stopping a process this way?

I'm not an expert, but I'm told that it can have side-effects. It also feels much less elegant, so I'd very much prefer a solution that does not kill the process by default :)

Also in your tests on Linux, does sending SIGINT with the currently released version of imageio-ffmpeg even do anything? It didn't seem to work when I tried it.

IIRC it did on some systems but not all.

[christopherhesse]

The code looks a lot more elegant. If this was a process that was writing, then I doubt we could just kill it. It looks like this code is just for reading things though, so I'm not sure what the downside is to doing SIGKILL here.

[almarklein]

I suppose that in theory, it should be fine to kill the process. But that may depend on what ffmpeg is doing, but it's still generally better practice to try and stop a process the right way.

Could you please try using p.stdin.write("q") and p.stind.close() instead of p.communicate() to see if that works? I would strongly prefer that over a kill.

[christopherhesse]

That does seem to work, I've updated the PR, thanks!

[almarklein]

That would have to be p.stdin.write(b"q") (the argument must be bytes), my mistake. Are you using Python 2, otherwise it would be weird if it worked :)

[christopherhesse]

Oh, well it looks like there's an except Exception around that block. Maybe it failed and then immediately fell back to kill()

[almarklein]

Haha, ah right :D

[christopherhesse]

I tried the bytes version and it didn't hang yet.

[christopherhesse]

But the tests indicate that it's still being killed.

[christopherhesse]

So my guess is that the new method doesn't work, but it once again falls back to kill() which does work.

[christopherhesse]

I think the p.stdin.send(b"q") doesn't ever successfully kill the process btw, sort of like the SIGINT method on my linux system (ubuntu 16.04).

[almarklein]

Ok, I looked into this a bit deeper, and my suggestion of using p.stdin.send(b"q") is a bad idea, and does not work since it does not flush the pipes. I keep getting back to the fact that p.communicate() should just work, with or without a timeout.

You said that p.communicate(b"q", timeout=0.5) would hang, which is weird. I came across this which suggests that the callee process may be failing to close the pipes. In other words, I think it might be a problem on your system. Can you please share a bit more info on it? E.g. what version of ffmpeg, and any specifics on how you're using it?

[christopherhesse]

Ubuntu 16.04.5 LTS imageio-ffmpeg 0.3.0 imageio 2.5.0 python 3.7.3

I'll see if I still get the issue when running a simpler script.

[christopherhesse]

Does imageio work if I use the library from multiple threads?

[christopherhesse]

The following script doesn't hang, but does segfault fairly often:

import imageio
from glob import glob
import sys
import os
import queue
import threading

def make_iterator(uris, q):
    while True:
        for uri in uris:
            r = imageio.get_reader(uri=uri, format="ffmpeg")
            q.put(r.get_next_data())

def main():
    num_threads = 16

    uris = glob(os.path.join(sys.argv[1], "*/*.mp4"))
    q = queue.Queue()
    threads = []
    for i in range(num_threads):
        t = threading.Thread(target=make_iterator, args=(uris, q))
        t.daemon = True
        t.start()
        threads.append(t)

    for i in range(100000):
        print(i)
        q.get()

if __name__ == "__main__":
    main()

[almarklein]

Threading should be fine. That code sample works fine for me (I let it run until 3453 iters, on Windows though).

[christopherhesse]

That should be enough, this is making me think there is something busted with this version of python or this linux system.

[christopherhesse]

The segfault issue could be unrelated, but it's awfully suspicious.

[christopherhesse]

I'll file a new issue for the segfault, this seems worth investigating first in case it's related to this hang issue.

[christopherhesse]

Thanks for being so responsive about these issues!

[christopherhesse]

Okay, filed https://github.com/imageio/imageio-ffmpeg/issues/20 please try it out and let me know what you think.

[almarklein]

Let's merge this and see how it goes ...

[almarklein]

mmm, this fix makes that ffmpeg is killed on Windows. Maybe decide what to do depending on the platform ...

[almarklein]

And on Linux too .. mmm