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

asyncio: support multiprocessing (support fork) #66285

Closed
danoreilly mannequin opened this issue Jul 26, 2014 · 30 comments · Fixed by #99539 or #99769
Closed

asyncio: support multiprocessing (support fork) #66285

danoreilly mannequin opened this issue Jul 26, 2014 · 30 comments · Fixed by #99539 or #99769
Assignees
Labels
3.12 bugs and security fixes topic-asyncio topic-multiprocessing type-feature A feature request or enhancement

Comments

@danoreilly
Copy link
Mannequin

danoreilly mannequin commented Jul 26, 2014

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

    @danoreilly
    Copy link
    Mannequin Author

    danoreilly mannequin commented Jul 26, 2014

    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.

    @danoreilly danoreilly mannequin added topic-asyncio type-bug An unexpected behavior, bug, or error labels Jul 26, 2014
    @danoreilly danoreilly mannequin changed the title _UnixDefaultEventLoop policy should either create a new loop or explicilty fail when get_event_loop() is called from a multiprocessing child process _UnixDefaultEventLoopPolicy should either create a new loop or explicilty fail when get_event_loop() is called from a multiprocessing child process Jul 26, 2014
    @gvanrossum
    Copy link
    Member

    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
    Copy link
    Mannequin Author

    danoreilly mannequin commented Jul 26, 2014

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

    @gvanrossum
    Copy link
    Member

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

    @vstinner
    Copy link
    Member

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

    @danoreilly
    Copy link
    Mannequin Author

    danoreilly mannequin commented Jul 27, 2014

    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
    Copy link
    Mannequin Author

    danoreilly mannequin commented Jul 27, 2014

    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
    Copy link
    Member

    A simple pid check in the policy should be enough.

    @danoreilly
    Copy link
    Mannequin Author

    danoreilly mannequin commented Jul 27, 2014

    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
    Copy link
    Mannequin Author

    danoreilly mannequin commented Sep 10, 2014

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

    @vstinner
    Copy link
    Member

    vstinner commented Feb 4, 2015

    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 vstinner changed the title _UnixDefaultEventLoopPolicy should either create a new loop or explicilty fail when get_event_loop() is called from a multiprocessing child process asyncio: support multiprocessing Feb 4, 2015
    @vstinner
    Copy link
    Member

    vstinner commented Feb 4, 2015

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

    @vstinner vstinner changed the title asyncio: support multiprocessing asyncio: support multiprocessing (support fork=) Feb 15, 2015
    @vstinner vstinner changed the title asyncio: support multiprocessing (support fork=) asyncio: support multiprocessing (support fork) Feb 15, 2015
    @thehesiod
    Copy link
    Mannequin

    thehesiod mannequin commented Feb 22, 2017

    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
    Copy link
    Member

    pitrou commented Jun 28, 2017

    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
    Copy link
    Member

    1st1 commented Jun 28, 2017

    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
    Copy link
    Member

    pitrou commented Jun 28, 2017

    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
    Copy link
    Member

    1st1 commented Jun 28, 2017

    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
    Copy link
    Member

    1st1 commented May 29, 2018

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

    @miss-islington
    Copy link
    Contributor

    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
    Copy link
    Member

    1st1 commented May 29, 2018

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

    @1st1
    Copy link
    Member

    1st1 commented May 29, 2018

    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
    Copy link
    Member

    1st1 commented May 30, 2018

    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
    Copy link
    Member

    1st1 commented May 30, 2018

    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

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @gvanrossum
    Copy link
    Member

    gvanrossum commented Oct 24, 2022

    This is the longest-open asyncio bug -- it dates from 2014! It was almost fixed in 2018 but the fix was rolled back because @1st1 wanted to do it "properly". Alas, that seemed too ambitious a plan, and it was never heard from again (unless a fix was committed without reference to this issue).

    @kumaraditya303
    Copy link
    Contributor

    I'll fix this in 3.12 but this won't be backported to older branches. I'll write up the details for implementation here, but I'll keep things simple. For now I am thinking of when the event loop is created, to register a fork handler to close the wakeup fd, any sockets e.g self-pipe and not share the event loop.

    @kumaraditya303 kumaraditya303 added the 3.12 bugs and security fixes label Nov 15, 2022
    @kumaraditya303 kumaraditya303 self-assigned this Nov 16, 2022
    @kumaraditya303
    Copy link
    Contributor

    kumaraditya303 commented Nov 17, 2022

    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
    Copy link
    Member

    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
    Copy link
    Member

    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
    Copy link
    Contributor

    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.

    kumaraditya303 added a commit that referenced this issue Nov 24, 2022
    `asyncio` now does not shares event loop and signal wakeupfd in forked processes.
    @gvanrossum gvanrossum reopened this Nov 24, 2022
    gvanrossum pushed a commit that referenced this issue Nov 24, 2022
    )
    
    Such buildbots (at the time of writing, only "AMD64 RHEL8 FIPS Only Blake2 Builtin Hash 3.x") cannot use multiprocessing with a fork server, so just skip the test there.
    @gvanrossum
    Copy link
    Member

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

    kumaraditya303 added a commit to kumaraditya303/cpython that referenced this issue Nov 24, 2022
    kumaraditya303 added a commit to kumaraditya303/cpython that referenced this issue Nov 24, 2022
    kumaraditya303 added a commit to kumaraditya303/cpython that referenced this issue Nov 25, 2022
    python#99745)
    
    Such buildbots (at the time of writing, only "AMD64 RHEL8 FIPS Only Blake2 Builtin Hash 3.x") cannot use multiprocessing with a fork server, so just skip the test there.
    @kumaraditya303 kumaraditya303 linked a pull request Nov 26, 2022 that will close this issue
    kumaraditya303 added a commit that referenced this issue Nov 27, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.12 bugs and security fixes topic-asyncio topic-multiprocessing type-feature A feature request or enhancement
    Projects
    Status: Done
    6 participants