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

Signaling an asyncio subprocess might raise ProcessLookupError, even if you haven't called .wait() yet #88319

Closed
syntaxcoloring mannequin opened this issue May 16, 2021 · 2 comments
Labels
topic-asyncio type-bug An unexpected behavior, bug, or error

Comments

@syntaxcoloring
Copy link
Mannequin

syntaxcoloring mannequin commented May 16, 2021

BPO 44153
Nosy @asvetlov, @1st1

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 2021-05-16.22:24:45.281>
labels = ['type-bug', '3.7', '3.10', 'expert-asyncio']
title = "Signaling an asyncio subprocess might raise ProcessLookupError, even if you haven't called .wait() yet"
updated_at = <Date 2021-05-16.22:28:20.661>
user = 'https://bugs.python.org/syntaxcoloring'

bugs.python.org fields:

activity = <Date 2021-05-16.22:28:20.661>
actor = 'syntaxcoloring'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['asyncio']
creation = <Date 2021-05-16.22:24:45.281>
creator = 'syntaxcoloring'
dependencies = []
files = []
hgrepos = []
issue_num = 44153
keywords = []
message_count = 1.0
messages = ['393764']
nosy_count = 3.0
nosy_names = ['asvetlov', 'yselivanov', 'syntaxcoloring']
pr_nums = []
priority = 'normal'
resolution = None
stage = None
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue44153'
versions = ['Python 3.7', 'Python 3.10']

@syntaxcoloring
Copy link
Mannequin Author

syntaxcoloring mannequin commented May 16, 2021

# Summary

Basic use of asyncio.subprocess.Process.terminate() can raise a ProcessLookupError, depending on the timing of the subprocess's exit.

I assume (but haven't checked) that this problem extends to .kill() and .send_signal().

This breaks the expected POSIX semantics of signaling and waiting on a process. See the "Expected behavior" section.

# Test case

I've tested this on macOS 11.2.3 with Python 3.7.9 and Python 3.10.0a7, both installed via pyenv.

import asyncio
import sys

# Tested with:
# asyncio.ThreadedChildWatcher (3.10.0a7  only)
# asyncio.MultiLoopChildWatcher (3.10.0a7 only)
# asyncio.SafeChildWatcher (3.7.9 and 3.10.0a7)
# asyncio.FastChildWatcher (3.7.9 and 3.10.0a7)
# Not tested with asyncio.PidfdChildWatcher because I'm not on Linux.
WATCHER_CLASS = asyncio.FastChildWatcher

async def main():
    # Dummy command that should be executable cross-platform.
    process = await asyncio.subprocess.create_subprocess_exec(
        sys.executable, "--version"
    )
    
    for i in range(20):
        # I think the problem is that the event loop opportunistically wait()s
        # all outstanding subprocesses on its own. Do a bunch of separate
        # sleep() calls to give it a bunch of chances to do this, for reliable
        # reproduction.
        #
        # I'm not sure if this is strictly necessary for the problem to happen.
        # On my machine, the problem also happens with a single sleep(2.0).
        await asyncio.sleep(0.1)
    
    process.terminate() # This unexpectedly errors with ProcessLookupError.

    print(await process.wait())

asyncio.set_child_watcher(WATCHER_CLASS())
asyncio.run(main())

The process.terminate() call raises a ProcessLookupError:

Traceback (most recent call last):
  File "kill_is_broken.py", line 29, in <module>
    asyncio.run(main())
  File "/Users/maxpm/.pyenv/versions/3.7.9/lib/python3.7/asyncio/runners.py", line 43, in run
    return loop.run_until_complete(main)
  File "/Users/maxpm/.pyenv/versions/3.7.9/lib/python3.7/asyncio/base_events.py", line 587, in run_until_complete
    return future.result()
  File "kill_is_broken.py", line 24, in main
    process.terminate() # This errors with ProcessLookupError.
  File "/Users/maxpm/.pyenv/versions/3.7.9/lib/python3.7/asyncio/subprocess.py", line 131, in terminate
    self._transport.terminate()
  File "/Users/maxpm/.pyenv/versions/3.7.9/lib/python3.7/asyncio/base_subprocess.py", line 150, in terminate
    self._check_proc()
  File "/Users/maxpm/.pyenv/versions/3.7.9/lib/python3.7/asyncio/base_subprocess.py", line 143, in _check_proc
    raise ProcessLookupError()
ProcessLookupError

# Expected behavior and discussion

Normally, with POSIX semantics, the wait() syscall tells the operating system that we won't send any more signals to that process, and that it's safe for the operating system to recycle that process's PID. This comment from Jack O'Connor on another issue explains it well: https://bugs.python.org/issue40550#msg382427

So, I expect that on any given asyncio.subprocess.Process, if I call .terminate(), .kill(), or .send_signal() before I call .wait(), then:

  • It should not raise a ProcessLookupError.
  • The asyncio internals shouldn't do anything with a stale PID. (A stale PID is one that used to belong to our subprocess, but that we've since consumed through a wait() syscall, allowing the operating system to recycle it).

asyncio internals are mostly over my head. But I think the problem is that the event loop opportunistically calls the wait() syscall on our child processes. So, as implemented, there's a race condition. If the event loop's wait() syscall happens to come before my .terminate() call, my .terminate() call will raise a ProcessLookupError.

So, as a corollary to the expectations listed above, I think the implementation details should be either:

  • Ideally, the asyncio internals should not call syscall wait() on a process until I call wait() on that process.
  • Failing that, .terminate(), .kill() and .send_signal() should should no-op if the asyncio internals have already called .wait() on that process.

@syntaxcoloring syntaxcoloring mannequin added 3.7 (EOL) end of life 3.10 only security fixes topic-asyncio type-bug An unexpected behavior, bug, or error labels May 16, 2021
@syntaxcoloring syntaxcoloring mannequin changed the title Signaling an asyncio subprocess raises ProcessLookupError, depending on timing Signaling an asyncio subprocess might raise ProcessLookupError, even if you haven't called .wait() yet May 16, 2021
@syntaxcoloring syntaxcoloring mannequin changed the title Signaling an asyncio subprocess raises ProcessLookupError, depending on timing Signaling an asyncio subprocess might raise ProcessLookupError, even if you haven't called .wait() yet May 16, 2021
@ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
@ezio-melotti ezio-melotti moved this to Todo in asyncio Jul 17, 2022
@kumaraditya303 kumaraditya303 removed 3.10 only security fixes 3.7 (EOL) end of life labels Oct 18, 2022
@kumaraditya303
Copy link
Contributor

Duplicate of #87744

@kumaraditya303 kumaraditya303 marked this as a duplicate of #87744 Oct 23, 2022
@kumaraditya303 kumaraditya303 closed this as not planned Won't fix, can't repro, duplicate, stale Oct 23, 2022
Repository owner moved this from Todo to Done in asyncio Oct 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic-asyncio type-bug An unexpected behavior, bug, or error
Projects
Status: Done
Development

No branches or pull requests

1 participant