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

With asyncio subprocess, send_signal() and the child process watcher will both call waitpid() #87744

Closed
lincheney mannequin opened this issue Mar 21, 2021 · 8 comments · May be fixed by #127051
Closed

With asyncio subprocess, send_signal() and the child process watcher will both call waitpid() #87744

lincheney mannequin opened this issue Mar 21, 2021 · 8 comments · May be fixed by #127051
Labels
3.13 bugs and security fixes 3.14 new features, bugs and security fixes topic-asyncio type-bug An unexpected behavior, bug, or error

Comments

@lincheney
Copy link
Mannequin

lincheney mannequin commented Mar 21, 2021

BPO 43578
Nosy @asvetlov, @1st1, @lincheney

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-03-21.06:53:15.574>
labels = ['type-bug', '3.9', 'expert-asyncio']
title = 'With asyncio subprocess, send_signal() and the child process watcher will both call waitpid()'
updated_at = <Date 2021-03-21.06:53:15.574>
user = 'https://github.com/lincheney'

bugs.python.org fields:

activity = <Date 2021-03-21.06:53:15.574>
actor = 'lincheney'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['asyncio']
creation = <Date 2021-03-21.06:53:15.574>
creator = 'lincheney'
dependencies = []
files = []
hgrepos = []
issue_num = 43578
keywords = []
message_count = 1.0
messages = ['389218']
nosy_count = 3.0
nosy_names = ['asvetlov', 'yselivanov', 'lincheney']
pr_nums = []
priority = 'normal'
resolution = None
stage = None
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue43578'
versions = ['Python 3.9']

Linked PRs

@lincheney
Copy link
Mannequin Author

lincheney mannequin commented Mar 21, 2021

Under unix, when creating a asyncio subprocess, the child process watcher will call waitpid() to reap the child, but if you call send_signal() (or terminate() or kill() ) on the asyncio subprocess, this will also call waitpid(), causing exactly one of these to fail, as you cannot call waitpid() on a PID more than once.

If the send_signal() fails, this doesn't seem much of an issue.
If the child process watcher fails however, it sets the returncode to 255 and also returns 255 when running wait() and also emits a warning.

I've seen this behaviour with the ThreadedChildWatcher, but possibly other Unix child watchers that use waitpid() suffer from the same problem.

The behaviour is racey (it depends on which one completes the waitpid() first), but if you try it enough it will appear:

import asyncio
import signal

async def main():
    while True:
        proc = await asyncio.create_subprocess_exec('sleep', '0.1')
        await asyncio.sleep(0.1)
        try:
            proc.send_signal(signal.SIGUSR1)
        except ProcessLookupError:
            pass
        assert (await proc.wait() != 255)

asyncio.run(main())

The output looks like:

Unknown child process pid 1394331, will report returncode 255
Traceback (most recent call last):
  File "/tmp/bob.py", line 14, in <module>
    asyncio.run(main())
  File "/usr/lib/python3.9/asyncio/runners.py", line 44, in run
    return loop.run_until_complete(main)
  File "/usr/lib/python3.9/asyncio/base_events.py", line 642, in run_until_complete
    return future.result()
  File "/tmp/bob.py", line 12, in main
    assert (await proc.wait() != 255)
AssertionError

This would be expected behaviour if I were explicitly calling waitpid() myself (ie I'm shooting my own foot, so I'd deserve the bad behaviour), but that's not the case here nor any other exotic code.

@lincheney lincheney mannequin added 3.9 only security fixes topic-asyncio type-bug An unexpected behavior, bug, or error labels Mar 21, 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
@FabianElsmer
Copy link

FabianElsmer commented Oct 20, 2022

Did you find a workaround for this? I tried the different watcher classes asyncio provides using your example script, but all of them show this error after a while

@gvanrossum
Copy link
Member

gvanrossum commented Oct 20, 2022

This one is new to me. I’m not sure how to approach it yet. The only workaround I can think of without digging in the code would be to ignore the error using try/except, but that’s a last resort.

@gvanrossum
Copy link
Member

@graingert Any idea here?

@gvanrossum
Copy link
Member

Oops, I see the problem isn't an exception so much as a lost exit status. I'm guessing we need to improve the communication between asyncio.subprocess.Process.wait() and the child watchers.

@kumaraditya303
Copy link
Contributor

On my TODO list but low priority, I can review a PR fixing this though.

@kumaraditya303 kumaraditya303 added 3.11 only security fixes 3.10 only security fixes 3.12 bugs and security fixes and removed 3.9 only security fixes labels Nov 15, 2022
@kumaraditya303
Copy link
Contributor

Child watcher will be removed in 3.14 at which point this will have to be reworked again so I'll fix this after child watchers are removed, it will be much simpler then.

@kumaraditya303 kumaraditya303 removed 3.11 only security fixes 3.10 only security fixes 3.12 bugs and security fixes labels Mar 19, 2023
byllyfish added a commit to byllyfish/shellous that referenced this issue Nov 15, 2023
@willingc
Copy link
Contributor

Revisit after completion of #120804

kumaraditya303 added a commit that referenced this issue Jul 1, 2024
)

asyncio earlier relied on subprocess module to send signals to the process, this has some drawbacks one being that subprocess module unnecessarily calls waitpid on child processes and hence it races with asyncio implementation which internally uses child watchers. To mitigate this, now asyncio sends signals directly to the process without going through the subprocess on non windows systems. On Windows it fallbacks to subprocess module handling but on windows there are no child watchers so this issue doesn't exists altogether.
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Jul 1, 2024
…pythonGH-121126)

asyncio earlier relied on subprocess module to send signals to the process, this has some drawbacks one being that subprocess module unnecessarily calls waitpid on child processes and hence it races with asyncio implementation which internally uses child watchers. To mitigate this, now asyncio sends signals directly to the process without going through the subprocess on non windows systems. On Windows it fallbacks to subprocess module handling but on windows there are no child watchers so this issue doesn't exists altogether.
(cherry picked from commit bd473aa)

Co-authored-by: Kumar Aditya <[email protected]>
@kumaraditya303 kumaraditya303 added 3.13 bugs and security fixes 3.14 new features, bugs and security fixes labels Jul 1, 2024
kumaraditya303 added a commit that referenced this issue Jul 1, 2024
GH-121126) (#121194)

gh-87744: fix waitpid race while calling send_signal in asyncio (GH-121126)

asyncio earlier relied on subprocess module to send signals to the process, this has some drawbacks one being that subprocess module unnecessarily calls waitpid on child processes and hence it races with asyncio implementation which internally uses child watchers. To mitigate this, now asyncio sends signals directly to the process without going through the subprocess on non windows systems. On Windows it fallbacks to subprocess module handling but on windows there are no child watchers so this issue doesn't exists altogether.

(cherry picked from commit bd473aa)

Co-authored-by: Kumar Aditya <[email protected]>
@github-project-automation github-project-automation bot moved this from Todo to Done in asyncio Jul 1, 2024
Akasurde pushed a commit to Akasurde/cpython that referenced this issue Jul 3, 2024
…python#121126)

asyncio earlier relied on subprocess module to send signals to the process, this has some drawbacks one being that subprocess module unnecessarily calls waitpid on child processes and hence it races with asyncio implementation which internally uses child watchers. To mitigate this, now asyncio sends signals directly to the process without going through the subprocess on non windows systems. On Windows it fallbacks to subprocess module handling but on windows there are no child watchers so this issue doesn't exists altogether.
noahbkim pushed a commit to hudson-trading/cpython that referenced this issue Jul 11, 2024
…python#121126)

asyncio earlier relied on subprocess module to send signals to the process, this has some drawbacks one being that subprocess module unnecessarily calls waitpid on child processes and hence it races with asyncio implementation which internally uses child watchers. To mitigate this, now asyncio sends signals directly to the process without going through the subprocess on non windows systems. On Windows it fallbacks to subprocess module handling but on windows there are no child watchers so this issue doesn't exists altogether.
estyxx pushed a commit to estyxx/cpython that referenced this issue Jul 17, 2024
…python#121126)

asyncio earlier relied on subprocess module to send signals to the process, this has some drawbacks one being that subprocess module unnecessarily calls waitpid on child processes and hence it races with asyncio implementation which internally uses child watchers. To mitigate this, now asyncio sends signals directly to the process without going through the subprocess on non windows systems. On Windows it fallbacks to subprocess module handling but on windows there are no child watchers so this issue doesn't exists altogether.
gschaffner added a commit to gschaffner/cpython that referenced this issue Nov 20, 2024
…_signal in asyncio (python#121126)"

This reverts the non-test changes of commit bd473aa.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.13 bugs and security fixes 3.14 new features, bugs and security fixes topic-asyncio type-bug An unexpected behavior, bug, or error
Projects
Status: Done
4 participants