In run_process moved deliver_cancel exception handling to the caller code

[guilledk]

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

[oremanj]

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.

[oremanj]

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.

[oremanj]

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]

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]

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]

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