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

Added more process arguments #749

Merged
merged 23 commits into from
Sep 3, 2024
Merged

Added more process arguments #749

merged 23 commits into from
Sep 3, 2024

Conversation

agronholm
Copy link
Owner

@agronholm agronholm commented Jun 23, 2024

Changes

This adds a number of new arguments to run_process() and open_process():

  1. startupinfo
  2. creationflags
  3. user
  4. group
  5. extra_groups
  6. umask

Fixes #742.

Checklist

If this is a user-facing code change, like a bugfix or a new feature, please ensure that
you've fulfilled the following conditions (where applicable):

  • You've added tests (in tests/) added which would fail without your patch
  • You've updated the documentation (in docs/, in case of behavior changes or new
    features)
  • You've added a new changelog entry (in docs/versionhistory.rst).

If this is a trivial change, like a typo fix or a code reformatting, then you can ignore
these instructions.

Updating the changelog

If there are no entries after the last release, use **UNRELEASED** as the version.
If, say, your patch fixes issue #123, the entry should look like this:

* Fix big bad boo-boo in task groups (#123 <https://github.com/agronholm/anyio/issues/123>_; PR by @yourgithubaccount)

If there's no issue linked, just link to your pull request instead by updating the
changelog after you've created the PR.

@agronholm agronholm changed the title More process kwargs Added more process arguments Jun 23, 2024
Copy link

@dolamroth dolamroth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@dolamroth
Copy link

dolamroth commented Jun 26, 2024

Tests (I copypasted function name limit_virtual_memory, but it actually limits CPU instead, since it's easier to test)

OS: CentOS Linux 8
Python 3.10.7

1.py

import sys
sys.setrecursionlimit(1000)

def fib(n):
    if n < 2:
        return n
    return fib(n-1) + fib(n-2)

print(fib(200))
sys.exit(0)

test.py

import anyio
import subprocess
import resource
import sys

def limit_virtual_memory():
    resource.setrlimit(resource.RLIMIT_CPU, (1, 1))

async def main():
    try:
        proc = await anyio.run_process([sys.executable, "1.py"], preexec_fn=limit_virtual_memory, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
        print(proc.stdout)
    except BaseException as exc:
        print(exc)


if __name__ == "__main__":
    anyio.run(main)

test_sync.py

import subprocess
import resource
import sys

def limit_virtual_memory():
    resource.setrlimit(resource.RLIMIT_CPU, (1, 1))

def main():
    try:
        proc = subprocess.run([sys.executable, "1.py"], preexec_fn=limit_virtual_memory, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
        print(proc.stdout)
    except BaseException as exc:
        print(exc)


if __name__ == "__main__":
    main()

test.py prints:

Command '['/home/user/venv/bin/python', '1.py']' died with <Signals.SIGKILL: 9>.

test_sync.py prints:

b''

@dolamroth
Copy link

dolamroth commented Jun 26, 2024

Test 2: switch back to limiting RAM

import anyio
import subprocess
import resource
import sys

MAX_VIRTUAL_MEMORY = 10 * 1024 * 1024


def limit_virtual_memory():
    resource.setrlimit(resource.RLIMIT_AS, (MAX_VIRTUAL_MEMORY, MAX_VIRTUAL_MEMORY))

async def main():
    try:
        proc = await anyio.run_process([sys.executable, "1.py"], preexec_fn=limit_virtual_memory, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
        print(proc.stdout)
    except BaseException as exc:
        print(exc)

if __name__ == "__main__":
    anyio.run(main)

If I set MAX_VIRTUAL_MEMORY = 10 * 1024, test.py outputs:

Command '['/home/user/venv/bin/python', '1.py']' died with <Signals.SIGSEGV: 11>.

but if I set it to a larger number MAX_VIRTUAL_MEMORY = 10 * 1024 * 1024, it for some reason outputs:

Command '['/home/user/venv/bin/python', '1.py']' returned non-zero exit status 127.

This doesn't happen in the same setup with test_sync.py (outputs b'')

@dolamroth
Copy link

dolamroth commented Jun 26, 2024

Test of trio

import trio
import subprocess
import resource
import sys
from functools import partial

MAX_VIRTUAL_MEMORY = 10 * 1024 * 1024

def limit_virtual_memory():
    resource.setrlimit(resource.RLIMIT_AS, (MAX_VIRTUAL_MEMORY, MAX_VIRTUAL_MEMORY))

async def main():
    try:
        async with trio.open_nursery() as nursery:
            start_proc = partial(trio.run_process, preexec_fn=limit_virtual_memory, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
            proc = await nursery.start(start_proc, [sys.executable, "1.py"])
        print(proc.stdout)
    except BaseException as exc:
        print(exc.exceptions)

if __name__ == "__main__":
    trio.run(main)

Outputs

(CalledProcessError(127, ['/home/user/venv/bin/python', '1.py']),)

@dolamroth
Copy link

dolamroth commented Jun 26, 2024

test_asyncio.py

import asyncio
import subprocess
import resource
import sys
from functools import partial


MAX_VIRTUAL_MEMORY = 10 * 1024 * 1024

def limit_virtual_memory():
    resource.setrlimit(resource.RLIMIT_AS, (MAX_VIRTUAL_MEMORY, MAX_VIRTUAL_MEMORY))

async def main():
    try:
        proc = await asyncio.create_subprocess_exec(*[sys.executable, "1.py"], preexec_fn=limit_virtual_memory, stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.PIPE)
        rc = await proc.wait()
        print(rc)
        print(proc.stdout)
    except BaseException as exc:
        print(exc)

if __name__ == "__main__":
    asyncio.run(main())

Outputs

127
<StreamReader eof transport=<_UnixReadPipeTransport closed fd=6 closed>>

src/anyio/_core/_subprocesses.py Show resolved Hide resolved
src/anyio/_core/_subprocesses.py Outdated Show resolved Hide resolved
src/anyio/abc/_eventloop.py Outdated Show resolved Hide resolved
src/anyio/_core/_subprocesses.py Show resolved Hide resolved
src/anyio/abc/_eventloop.py Outdated Show resolved Hide resolved
src/anyio/abc/_eventloop.py Outdated Show resolved Hide resolved
docs/versionhistory.rst Outdated Show resolved Hide resolved
@gschaffner
Copy link
Collaborator

Comparing to the full list of Popen parameters:

# Supported:
args,
stdin,
stdout,
stderr,
cwd,
env,
startupinfo,
creationflags,
start_new_session,
pass_fds,
user,
group,
extra_groups,
umask,

# Not supported:
preexec_fn,  # Barely supported by Python (unsafe with threads)
close_fds,  # Unsafe (just set it to True and have users use `pass_fds` and `startupinfo.lpAttributeList["handle_list"]` instead)
# Not supported by asyncio:
bufsize,
universal_newlines,
shell,
text,
encoding,
errors,
# _Possible_ remaining contenders to support in AnyIO (AFAIK nobody has requested any of
# these yet):
process_group,  # Similar to `start_new_session` in that it can replace `preexec_fn` use cases.
executable,
restore_signals,
pipesize,  # Not super useful since if using pipes you should have tasks always reading from the std{out,err} streams.

Personally I do not have a use-case for any of those four at the bottom right now, but I wanted to ask why you excluded them.

@agronholm
Copy link
Owner Author

Comparing to the full list of Popen parameters:

# Supported:
args,
stdin,
stdout,
stderr,
cwd,
env,
startupinfo,
creationflags,
start_new_session,
pass_fds,
user,
group,
extra_groups,
umask,

# Not supported:
preexec_fn,  # Barely supported by Python (unsafe with threads)
close_fds,  # Unsafe (just set it to True and have users use `pass_fds` and `startupinfo.lpAttributeList["handle_list"]` instead)
# Not supported by asyncio:
bufsize,
universal_newlines,
shell,
text,
encoding,
errors,
# _Possible_ remaining contenders to support in AnyIO (AFAIK nobody has requested any of
# these yet):
process_group,  # Similar to `start_new_session` in that it can replace `preexec_fn` use cases.
executable,
restore_signals,
pipesize,  # Not super useful since if using pipes you should have tasks always reading from the std{out,err} streams.

Personally I do not have a use-case for any of those four at the bottom right now, but I wanted to ask why you excluded them.

Many are outright dangerous, or problematic from the type annotations PoV. I tried to include a larger number of these but gave up on the annotations front.

@agronholm agronholm merged commit 2532e43 into master Sep 3, 2024
17 checks passed
@agronholm agronholm deleted the more-process-kwargs branch September 3, 2024 06:32
@agronholm
Copy link
Owner Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow passing kwargs to anyio.open_process and anyio.run_process
3 participants