asyncio: support multiprocessing (support fork)

[danoreilly]

BPO	22087
Nosy	@gvanrossum, @pitrou, @1st1, @thehesiod, @miss-islington
PRs	bpo-22087: Fix Policy.get_event_loop() to detect fork #7208 [3.7] bpo-22087: Fix Policy.get_event_loop() to detect fork (GH-7208) #7215 [3.6] bpo-22087: Fix Policy.get_event_loop() to detect fork (GH-7208) #7218 bpo-22087: Restructure code to be more robust and safe #7226 Revert "bpo-22087: Fix Policy.get_event_loop() to detect fork (GH-7208)" #7232 Revert "bpo-22087: Fix Policy.get_event_loop() to detect fork (GH-7208)" #7233
Files	test_loop.py: Test script demonstrating the issue handle_mp_unix.diff: Patch that makes _UnixDefaultEventLoopPolicy create a new loop object if get_event_loop is called in a forked mp child process handle-mp_unix2.patch: Use os.getpid() instead of multiprocessing. Store pid state in Policy instance rather than the Loop instance. handle_mp_unix_with_test.diff: Adds a unit test to previous patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2014-07-26.18:01:10.150>
labels = ['type-bug', 'expert-asyncio']
title = 'asyncio: support multiprocessing (support fork)'
updated_at = <Date 2018-05-30.00:56:36.541>
user = 'https://bugs.python.org/danoreilly'

bugs.python.org fields:

activity = <Date 2018-05-30.00:56:36.541>
actor = 'yselivanov'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['asyncio']
creation = <Date 2014-07-26.18:01:10.150>
creator = 'dan.oreilly'
dependencies = []
files = ['36117', '36118', '36119', '36134']
hgrepos = []
issue_num = 22087
keywords = ['patch']
message_count = 23.0
messages = ['224082', '224084', '224085', '224097', '224125', '224140', '224143', '224144', '224145', '226698', '235404', '235411', '288327', '297222', '297226', '297227', '297229', '318077', '318092', '318135', '318140', '318143', '318144']
nosy_count = 7.0
nosy_names = ['gvanrossum', 'pitrou', 'zmedico', 'yselivanov', 'thehesiod', 'dan.oreilly', 'miss-islington']
pr_nums = ['7208', '7215', '7218', '7226', '7232', '7233']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue22087'
versions = ['Python 3.4', 'Python 3.5', 'Python 3.6']

Linked PRs

• GH-66285: fix forking in asyncio #99539
• GH-66285: skip asyncio fork tests for platforms without md5 hash  #99745
• GH-66285: Revert "fix forking in asyncio" #99756
• GH-66285: fix forking in asyncio #99769
• GH-66285: remove redundant time.sleep from test_fork_signal_handling #99963

[danoreilly]

On non-Windows platforms, if a user attempts to use asyncio.get_event_loop() in a child process created by multiprocessing.Process using the fork context, and an asyncio event loop is also being used in the main process, the same _UnixSelectorEventLoop object will be used by both processes. This, of course, won't work properly; the child will raise a "RuntimeError: Event loop is running" exception as soon as it tries using the loop object.

However, this may or may not actually make it back to the parent: If the parent is expecting to get items from a queue from that child publishes to, rather than yielding from it immediately, the program will deadlock. Even if the child is yielded from, it may not be immediately obvious why "Event loop is running" was raised, and the behavior is inconsistent with the behavior if a method other than os.fork is used to create the child process, since the child will get a new event loop in that case.

So, it'd be better if _UnixDefaultEventLoopPolicy detected that get_event_loop was being called in a child process, and either

1. Created a new loop for the child (this would make the behavior appear consistent no matter what platform/method for launching children is used)
2. Raised an exception stating that no default event loop exists for this process, similar to the assert used for threads currently.

I've attached a test script that demonstrates the different between forked/spawned processes, and a patch that implements #1 above.

[gvanrossum]

Good point. Asyncio definitely should not share event loops across forked processes. However, I don't like the dependency on multiprocessing (even though it's in the stdlib) -- can't the policy just use os.getpid()?

Also, I've got a feeling that maybe the pid should be part of the policy state instead of the loop state? The policy could just reset self._local when the pid doesn't match.

[danoreilly]

Yep, agreed on both points. The latter suggestion also has the benefit of not requiring any test changes. Here's an updated patch.

[gvanrossum]

I think there should still be a new unittest -- we're adding a behavior we're promising, so we should test it.

[vstinner]

See aslo issue bpo-21998: "asyncio: a new self-pipe should be created in the child process after fork".

[danoreilly]

I've added a unit test that spawns a new forked process via multiprocessing, and verifies that the loop returned by get_event_loop is not the same as the one we have in the parent.

[danoreilly]

re: bpo-21998, perhaps it's time to revive bpo-16500? Without that, I'm not sure what can be done aside from documenting the need to call "loop = asyncio.get_event_loop()" in the child immediately after forking.

[vstinner]

A simple pid check in the policy should be enough.

[danoreilly]

Hmm, I'm not sure what you mean. What check in the policy would prevent this issue you described in bpo-21998?:

import asyncio, os
loop = asyncio.get_event_loop()
pid = os.fork()
if pid:
    print("parent", loop._csock.fileno(), loop._ssock.fileno())
else:
    print("child", loop._csock.fileno(), loop._ssock.fileno())

Output:
---
parent 6 5
child 6 5

[danoreilly]

Are any other changes needed here? I'm still not completely clear on what Victor meant with his last comment.

[vstinner]

This issue looks to be a duplicate of bpo-21998.

handle-mp_unix2.patch looks more to a workaround than a real issue. When I write asyncio code, I prefer to pass explicitly the loop, so get_event_loop() should never be called. IMO the methods of the event loop should detect the fork and handle the fork directly.

[vstinner]

See also the https://pypi.python.org/pypi/mpworker project

[thehesiod]

I believe this is now worse due to python/asyncio#452

before I was able to simply create a new run loop from sub-processes however you will now get the error "Cannot run the event loop while another loop is running". The state of the run loop should not be preserved in sub-processes either.

[pitrou]

The approach in Dan's patches looks sane to me and I agree the issue needs fixing. Dan, would you like to submit a PR for this? We're using Github for patch submissions now.

[1st1]

The approach in Dan's patches looks sane to me and I agree the issue needs fixing. Dan, would you like to submit a PR for this? We're using Github for patch submissions now.

Mind that the patch is very outdated and we already have these checks in get_event_loop.

[pitrou]

It seems there's a check in the top-level get_event_loop() function but not in the DefaultEventLoopPolicy.get_event_loop() method.

(also: wow, that stuff is complicated)

[1st1]

It seems there's a check in the top-level get_event_loop() function but not in the DefaultEventLoopPolicy.get_event_loop() method.

Yes, as users aren't supposed to work with policies directly.

(also: wow, that stuff is complicated)

Yep. Please assign me to review if you submit a PR.

[1st1]

New changeset 5d97b7b by Yury Selivanov in branch 'master':
bpo-22087: Fix Policy.get_event_loop() to detect fork (GH-7208)
5d97b7b

[miss-islington]

New changeset 2a7eb0b by Miss Islington (bot) in branch '3.7':
bpo-22087: Fix Policy.get_event_loop() to detect fork (GH-7208)
2a7eb0b

[1st1]

another related issue: https://bugs.python.org/issue33688

[1st1]

Even though I committed a version of Dan's patch to 3.7 and 3.8, I've finally decided to revert it and do it properly instead. We should strive to implement a proper solution, not commit some half-working code.

A concrete plan (for Python 3.8 probably):

1. Fix BaseDefaultEventLoopPolicy to track PID in its 'get_event_loop()' and 'set_event_loop()' methods. If a PID has changed since the last invocation: reset its internal local state.

2. Fix _UnixDefaultEventLoopPolicy to check for PID change in 'get_child_watcher()' and 'set_child_watcher()'.

3. Fix child watcher / event loops to track PID changes too to avoid listening for child processes of their parent process.

4. Look at how libuv and other event loops implement fork support. Ideally we should be able to shutdown selectors (epoll, kqueue) in forked processes in such a way that their parent process isn't affected.

5. Think how we can make 'asyncio.run' fork friendly. Ideally it should initialize its own child watcher and remove it when its done.

[1st1]

New changeset 99279ad by Yury Selivanov in branch 'master':
Revert "bpo-22087: Fix Policy.get_event_loop() to detect fork (GH-7208)" (bpo-7232)
99279ad

[1st1]

New changeset af9cda9 by Yury Selivanov in branch '3.7':
Revert "bpo-22087: Fix Policy.get_event_loop() to detect fork (GH-7208)" (GH-7233)
af9cda9

[kumaraditya303]

See #99539

The PR does two things with a fork handler:

• Reset the event loop registry so that child and parent process do not share the same event loop.
• Reset the wakeupfd in child so that when a signal is sent to child, the signal handler of the parent is not called.

I have added two tests, first tests that event loop is not shared across forked processes and the second tests that the wakeup fd is reset.

[gvanrossum]

Is the intention of this PR that after the fork() call the child can use all asyncio functionality and everything should work?

Maybe we should discuss what exactly we guarantee after this PR (and perhaps what guarantees already hold -- I presume if the forked child creates a brand new loop everything did work already except for signals?)

[gvanrossum]

In particular the PR's title seems to claim that everything related to forking and asyncio will be fixed. How much do you yourself believe that claim?

[kumaraditya303]

In particular the PR's title seems to claim that everything related to forking and asyncio will be fixed. How much do you yourself believe that claim?

Everything should work in forked process as it normally does without fork, I have added test for this in the latest commit.

[gvanrossum]

Let's keep this open until the buildbot results come in. Lukasz is bisecting them as we speak.