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

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

Conversation

guilledk
Copy link

@guilledk guilledk commented May 27, 2020

Hey guys, so the test for this change is failing, I can't seem to catch the OSError on trio/tests/test_subprocess.py:test_warn_on_failed_cancel_terminate.

I guess it's being raised in another "context" (it context the right word)?

Also any comments on the exception logging? Is trio.run_process the correct logger? Also what about the message?

Closes #1532

@guilledk guilledk requested a review from njsmith May 27, 2020 01:28
Copy link
Member

@oremanj oremanj left a comment

Choose a reason for hiding this comment

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

You'll need to add a newsfragment for this change.

I'm not immediately sure why the test is failing. You'll need to do some digging. If you remove the with pytest.raises() context manager, does an exception get raised out of the test? If not, can you add with pytest.raises() at some point closer to where the exception is raised, and see it successfully catch the exception? What does this tell you about how far the exception is making it? Or you can step through the failing test in a debugger, starting at the point where the exception is raised.

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.

@@ -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")

@oremanj
Copy link
Member

oremanj commented May 27, 2020

I'm not immediately sure why the test is failing. You'll need to do some digging.

I did some digging, and it's super subtle: #1559. Basically, the nursery in run_process() is surrounded by async with process:, and Process.aclose() raises Cancelled when it executed in a cancelled scope; this winds up with the error from the nursery getting stuck as the context of that Cancelled exception, which is swallowed by the cancel scope in the outermost nursery (in your test function).

I'm blanking on a fix right now. This is a much larger problem than just your diff -- see #455 for more discussion. @njsmith, any ideas?

@njsmith
Copy link
Member

njsmith commented May 27, 2020

Well, I guess there's an easy local fix... we already want to deprecate using Process objects as a context manager, so that will automatically take care of this :-) (#1104)

For the larger issue..... well, let's talk in #455/#1559.

@njsmith
Copy link
Member

njsmith commented May 28, 2020

To clarify: what we want to deprecate is the use of async with <process object>, which is the thing that's causing your test to fail.

That might also be the quickest way to get this working... we're already waiting for the process in all cases, so you could replace:

async with await trio.open_process(...) as proc:
    blah

with

proc = await trio.open_process(...)
blah

@guilledk guilledk requested a review from oremanj June 4, 2020 13:42
@guilledk guilledk closed this Jul 5, 2020
@guilledk guilledk reopened this Jul 5, 2020
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.

In run_process, what should happen if the deliver_cancel function raises an exception?
3 participants