Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Atfork #68

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Atfork #68

wants to merge 2 commits into from

Conversation

neilrahilly
Copy link

This fixes #80: https://bitbucket.org/eventlet/eventlet/issue/80/eexist-should-be-ignored-for-epoll. The issue is that a parent and child process can end up sharing an instance of epoll after a fork. Since file descriptors are unique only within a process, the two processes can end up with the same file descriptor pointing to two completely different resources. So the child registers socket A with FD 3 and the parent registers socket B with FD 3, and epoll gets messed up. These conflicts are likely because Linux will use the lowest free number for creating a new file descriptors.

We found this happening at Mixpanel in one of our services that's deployed using a prefork server. If some code causes the server process to create an eventlet hub prior to the fork, the master and workers all share the same epoll instance, and strange behavior ensues.

More generally, like threads, it doesn't make much sense to have a child process inherit its parent's state. Therefore, we decided to clean the parent's hub out of the child after fork. Any subsequent use of eventlet in the child will generate a new hub.

."

This reverts commit 2dd4148.

Conflicts:

	tests/hub_test.py
this is particularly important for epoll and kqueue hubs. if they wind
up being shared by parent and child processes after a fork, the two
processes's conflict with each other because they wind up using the same
file descriptors to refer to different resources. this change gives the
child process a fresh hub of its own after a fork.
@neilrahilly
Copy link
Author

@temoto please take a look when you have time. we use eventlet quite heavily at mixpanel and we'd like to get into the habit of upstreaming our changes and deploying using the public releases. thanks!

@temoto
Copy link
Member

temoto commented Dec 17, 2013

Thank you very much for contribution, @neilrahilly
The atfork module breaks os.fork name and docstring, but that's minor, I will fix that.

Any reason to not use existing eventlet/green/os.py ?

Could you explain why did you remove _kqueue_reinit?

@neilrahilly
Copy link
Author

Could go in eventlet/green/os.py. Would register_child_callback belong there as well?

_kqueue_reinit was how the kqueue hub solved this problem before. If a kqueue hub encountered an error from kqueue, it would check to see if the pid had changed--indicating execution was occurring in a forked child--and would reset the hub using _kqueue_reinit. The _del_hub atfork callback preemptively resets the hub in the child after a fork. It solves the problem for all hub types and makes the old _kqueue_reinit fix redundant. The exception that _kqueue_reinit was made to handle should no longer happen.

upd: markup

@temoto
Copy link
Member

temoto commented Dec 19, 2013

It would seem natural to have all patched versions of standard functions in one place. The only difference, that in this case you force patching of os.fork.

Thanks for explanation, this needs careful testing including Darwin and Freebsd before merge.

@temoto
Copy link
Member

temoto commented Dec 19, 2013

I'd put register_child_callback in green.os as well, just to keep related things together.

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.

Converting HTTP header name to lower case
2 participants