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

forkserver process isn't re-launched if it died #75489

Closed
pitrou opened this issue Aug 30, 2017 · 8 comments
Closed

forkserver process isn't re-launched if it died #75489

pitrou opened this issue Aug 30, 2017 · 8 comments
Labels
3.7 (EOL) end of life stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@pitrou
Copy link
Member

pitrou commented Aug 30, 2017

BPO 31308
Nosy @pitrou, @applio
PRs
  • bpo-31308: If multiprocessing's forkserver dies, launch it again when necessary #3246
  • [3.6] bpo-31308: If multiprocessing's forkserver dies, launch it again when necessary (GH-3246) #4252
  • 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 = <Date 2017-11-03.13:00:04.903>
    created_at = <Date 2017-08-30.14:27:15.775>
    labels = ['3.7', 'type-bug', 'library']
    title = "forkserver process isn't re-launched if it died"
    updated_at = <Date 2017-11-03.13:00:04.902>
    user = 'https://github.com/pitrou'

    bugs.python.org fields:

    activity = <Date 2017-11-03.13:00:04.902>
    actor = 'pitrou'
    assignee = 'none'
    closed = True
    closed_date = <Date 2017-11-03.13:00:04.903>
    closer = 'pitrou'
    components = ['Library (Lib)']
    creation = <Date 2017-08-30.14:27:15.775>
    creator = 'pitrou'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 31308
    keywords = ['patch']
    message_count = 8.0
    messages = ['301027', '301854', '301868', '303458', '304468', '305471', '305474', '305477']
    nosy_count = 3.0
    nosy_names = ['pitrou', 'davin', 'Matthew Rocklin']
    pr_nums = ['3246', '4252']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue31308'
    versions = ['Python 3.6', 'Python 3.7']

    @pitrou
    Copy link
    Member Author

    pitrou commented Aug 30, 2017

    It may happen that the forkserver process dies (for example if SIGINT is received because the user pressed Ctrl-C) while the parent process is still alive. In that case, if the parent tries to create a new Process instance, an exception is received.

    The exception looks like this:

    File "/xxx/lib/python3.5/multiprocessing/popen_forkserver.py", line 52, in _launch
    self.sentinel, w = forkserver.connect_to_new_process(self._fds)
    File "/xxx/lib/python3.5/multiprocessing/forkserver.py", line 66, in connect_to_new_process
    client.connect(self._forkserver_address)
    ConnectionRefusedError: [Errno 111] Connection refused

    @pitrou pitrou added 3.7 (EOL) end of life stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Aug 30, 2017
    @applio
    Copy link
    Member

    applio commented Sep 11, 2017

    I have two concerns with this:

    1. The implicit restart of the forkserver process seems in conflict with the zen of making things explicit.
    2. This would seem to make forkserver's behavior inconsistent with the behavior of things like the Manager which similarly creates its own process for managing resources but does not automatically restart that process if it should die or become unreachable. In the case of the Manager, I don't think we'd want it to automagically restart anything in these situations so it's not a simple matter of enhancing the Manager to adopt similar behavior.

    I do appreciate the use cases that would be addressed by having a convenient way to detect that a forkserver has died and then restart it. If the forkserver dies, I doubt we really want it to try to restart a potentially infinite number of times.

    Maybe a better path would be if we had a way to explicitly request that the Process trigger a restart of the forkserver, if necessary, but this setting/request defaults to False?

    @pitrou
    Copy link
    Member Author

    pitrou commented Sep 11, 2017

    1. The implicit restart of the forkserver process seems in conflict with the zen of making things explicit.

    Well, the forkserver process really is an implementation detail. It already is started on-demand, so restarting it if needed sounded like a natural evolution.

    Another example: in a multiprocessing Pool, if a worker process crashes, another will be restarted transparently (same in concurrent.futures.ProcessPoolExecutor, incidentally).

    1. This would seem to make forkserver's behavior inconsistent with the behavior of things like the Manager which similarly creates its own process for managing resources but does not automatically restart that process if it should die or become unreachable.

    That's possible. I've never seen the Manager used in the wild. Though if some fundamental difference justifies the inconsistency, the inconsistency is not really a problem.

    @pitrou
    Copy link
    Member Author

    pitrou commented Oct 1, 2017

    Looks like this will not make into 3.6.3.

    I'd like to sum up the discussion a bit:

    • I think at the very least least we must protect the forkserver against SIGINT, like the semaphore tracker already is
    • I think it's beneficial to also restart it on demand if necessary
    • I can't think of a use case where the user asks "no, please don't restart the forkserver on demand" as there is no way to start it manually anyway, and not restarting it breaks functionality

    Also, bpo-31310 is a similar issue for the semaphore tracker.

    @MatthewRocklin
    Copy link
    Mannequin

    MatthewRocklin mannequin commented Oct 16, 2017

    From a user's perspective I would definitely appreciate forkserver being protected from SIGINT. This bug affects me in practice. If the forkserver goes down I personally have no objection to it restarting automatically, though I appreciate that I have a narrow view of this topic.

    Davin, at your last comment it seemed like you had reservations about this going in. Did Antoine's recent comments resolve these concerns or no? Do you have any suggestions on what might be done to protect users from SIGINT crashing the forkserver?

    @pitrou
    Copy link
    Member Author

    pitrou commented Nov 3, 2017

    Davin, I think I'm going to merge the PR for this. If you object it, it can still be reverted later.

    @pitrou
    Copy link
    Member Author

    pitrou commented Nov 3, 2017

    New changeset fc6b348 by Antoine Pitrou in branch 'master':
    bpo-31308: If multiprocessing's forkserver dies, launch it again when necessary (bpo-3246)
    fc6b348

    @pitrou
    Copy link
    Member Author

    pitrou commented Nov 3, 2017

    New changeset 019c99f by Antoine Pitrou in branch '3.6':
    [3.6] bpo-31308: If multiprocessing's forkserver dies, launch it again when necessary (GH-3246) (bpo-4252)
    019c99f

    @pitrou pitrou closed this as completed Nov 3, 2017
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants