Fix - freeze when reading stdout from external process

[grungy-ado]

In one project I encounter with random freezing behavior when reading stdout from spawned external process. I start to suspect uvloop because using pure asyncio loop does not cause this problem.
Read from stdout was provided by external library that spawn new process using low level popen() call that seems to create new process by forking parent process. Debugger reveals that forked child process is hanged by trying to acquire GIL (that was strange because external process has nothing to do with python). Uvloop seems to register __atfork_child interceptor that add some logic in forking behavior that require GIL to be acquired. That make sense when fork is provided by uvloop, but not so much when fork is provided by anybody else.

I manage to replicate the problem in test case and hopefully fix it.

[pvizeli]

Should also fix the issue of #118

[grungy-ado]

Should also fix the issue of #118

If you are willing to provide some test case I can look at it. Probably in separate PR.

[1st1]

I'm not sure I follow... The old handler had a nogil decoration, i.e. cdef void __atfork_child() nogil:. How's this indirection makes that different?

[grungy-ado]

There is small but significant difference in case you compile it with cython and nogil traces enabled (-DCYTHON_TRACE=1 -DCYTHON_TRACE_NOGIL=1).
In this mode cython will insert trace calls during compilation and resulting compiled __atfork_child function will contain __Pyx_TraceCall. It is not a problem when fork contains python runtime, but it will freeze at this trace call in case forking child does not contains python runtime.

Traces are enabled in make debug, so I decide to write it in pure C where no such traces are inserted by cython.

[1st1]

Just so that I understand the scope: this PR is only needed to unbreak make debug?

[grungy-ado]

No, on the contrary, this PR should fix freezing uvloop (under some conditions) and freezing occurs even when compiled with pure make (without debug), but for a different reason.

[1st1]

Alright, I'll review the PR soon.

In the meantime, could you please squash all commits into 1 and write a commit message that explains the bug/problem in a detailed way? It would help me / future maintainers a lot.

Huge props for coming up with a functional test case.

[grungy-ado]

Thanks, I squash it. Give me a note if there is something not clear enough.

[elprans]

I don't fully understand why this change would fix the GIL issue. You are still calling the original __get_fork_handler which still grabs the GIL. The fact that you've wrapped the call into a C function shouldn't change that. What would make sense is to avoid installing __atfork_child globally, and only for the period when the loop is doing the fork.

[grungy-ado]

I don't fully understand why this change would fix the GIL issue. You are still calling the original __get_fork_handler which still grabs the GIL. The fact that you've wrapped the call into a C function shouldn't change that.

The fact that I've wrapped the call into a C function is the crucial part because decision if __get_fork_handler should be called or not (uvloop/includes/fork_handler.h:12) is in C function where no GIL is even present. That means that if forking is done by uvloop it is save to acquire GIL and call __get_fork_handler. On the other hand if forking is NOT provided by uvloop this condition is false, no python/cython function handler is called and child process continues uninterrupted.

In current implementation this decision is located inside cython function __atfork_child that always assume some python runtime is present and so before it make this crucial decision it tries to acquire GIL ( uvloop/loop.pyx:3125). But there are situations where you can fork and execute child process that has no python (means no GIL) as shown in provided test case (tests/test_process_spawning.py) where /bin/echo is being forked and freeze because trying to call __atfork_child without any python runtime that can acquire GIL. You can notice this if you run provided test case in gdb.

Now you may assume that solution can be as simple as mark __atfork_child as nogil and remove all GIL acquisitions, but that is not true either because if you use cython function and compile it in debug mode, cython will insert trace calls in each cdef function. In this solution you get into the same situation as before because inserted trace calls also assume that there is python runtime and sometimes also acquire GIL.

I hope my description is now more clear why this implementation should avoid using any cython function as a fork handler and why C function is much better choice.

What would make sense is to avoid installing __atfork_child globally, and only for the period when the loop is doing the fork.

I totally agree but currently pthread_atfork system call is used to register handler globally and there is no way (known to me) how to remove this handler. So the solution is to use fork handler to decide when loop is doing the fork. If you know about some other suitable system call that is capable to add and remove fork handler let me know and I will change it.

[1st1]

@grungy-ado I'm trying to run the unit test you added with unmodified uvloop on Linux and macOS and it doesn't fail. Any advice on how to make sure that the unit test actually works?

[grungy-ado]

@1st1 That's strange because it fails for me on my ubuntu 18.04LTS machine. It even failed in travis when I push only the test case #283 : https://travis-ci.org/MagicStack/uvloop/builds/601677156?utm_source=github_status&utm_medium=notification . Though it seems like linux issue only. There is no fail for OSX.

[pvizeli]

We had this issue also only on linux

[elprans]

there is no way (known to me) how to remove this handler

You're right, there seems to be no (portable) way to do this.

decision if __get_fork_handler should be called or not (uvloop/includes/fork_handler.h:12) is in C function where no GIL is even present

Yes, I see it now. I misread the diff initially. This makes sense, although it strikes me that those __forking_ globals should really be thread-local.

That's strange because it fails for me on my ubuntu 18.04LTS machine

I can reproduce on Ubuntu 18.04 and Debian Buster, but not on other distros like Gentoo or Alpine or even Ubuntu 19.10, so this definitely also depends on the version of libc.

[pvizeli]

but not on other distros like Gentoo or Alpine

It's possible if you run ffmpeg and also only random on Alpine.

[1st1]

Thanks so much for your patience and help, @grungy-ado. Merged. I'll do some cosmetic changes & refactorings but your approach is good.

[grungy-ado]

Great, thanks all