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.

Sign up for GitHub

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

Jump to bottom

In run_process moved deliver_cancel exception handling to the caller code #1555

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

In run_process moved deliver_cancel exception handling to the caller code #1555

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1f8e9a4
Started work on issue1532
May 27, 2020
4f554ac
Merge branch 'master' into issue1532
Jun 4, 2020
e3b2763
Repalced context managed process with simple open_process call, added…
Jun 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 19 additions & 19 deletions trio/_subprocess.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import os
import subprocess
import logging
import sys
from typing import Optional
from functools import partial
Expand Down Expand Up @@ -381,28 +382,20 @@ async def open_process(


async def _windows_deliver_cancel(p):
try:
p.terminate()
except OSError as exc:
warnings.warn(RuntimeWarning(f"TerminateProcess on {p!r} failed with: {exc!r}"))
p.terminate()


async def _posix_deliver_cancel(p):
try:
p.terminate()
await trio.sleep(5)
warnings.warn(
RuntimeWarning(
f"process {p!r} ignored SIGTERM for 5 seconds. "
f"(Maybe you should pass a custom deliver_cancel?) "
f"Trying SIGKILL."
)
)
p.kill()
except OSError as exc:
warnings.warn(
RuntimeWarning(f"tried to kill process {p!r}, but failed with: {exc!r}")
p.terminate()
await trio.sleep(5)
warnings.warn(
RuntimeWarning(
f"process {p!r} ignored SIGTERM for 5 seconds. "
f"(Maybe you should pass a custom deliver_cancel?) "
f"Trying SIGKILL."
)
)
p.kill()


async def run_process(
Expand Down Expand Up @@ -621,7 +614,14 @@ async def read_output(stream, chunks):

async def killer():
with killer_cscope:
await deliver_cancel(proc)
try:
await deliver_cancel(proc)
except BaseException as exc:
LOGGER = logging.getLogger("trio.run_process")
LOGGER.exception(
f"tried to kill process {proc!r}, but failed with: {exc!r}"
)
Copy link
Member

Choose a reason for hiding this comment

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

Dedent this paren to satisfy black. (It's often easier to run black setup.py trio before you commit than it is to predict what black will like.)

This should have failed the formatting check, but it looks like we're not properly rejecting PRs that require black changes. I'll address that in a separate PR.

raise

nursery.start_soon(killer)
await proc.wait()
Expand Down
2 changes: 1 addition & 1 deletion trio/tests/test_subprocess.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -464,7 +464,7 @@ def broken_terminate(self):

Copy link
Member

Choose a reason for hiding this comment

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

This is no longer testing a warning, so you should change the name of the test (since it's currently "warn_on_failed_cancel_terminate")

monkeypatch.setattr(Process, "terminate", broken_terminate)

with pytest.warns(RuntimeWarning, match=".*whoops.*"):
with pytest.raises(OSError, match=".*whoops.*"):
async with _core.open_nursery() as nursery:
nursery.start_soon(run_process, SLEEP(9999))
await wait_all_tasks_blocked()
Expand Down