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

Add nursery fixture #25

Closed
touilleMan opened this issue Dec 18, 2017 · 11 comments · Fixed by #26
Closed

Add nursery fixture #25

touilleMan opened this issue Dec 18, 2017 · 11 comments · Fixed by #26

Comments

@touilleMan
Copy link
Member

I'm currently seeing a big limitation to trio async fixtures: there is no nursery available when they are run.

Considering a usecase when you want to test the connection to a server, you have to:

  1. start the server
  2. connect to the server
  3. send a request to the server and check the result
  4. close the server

Without nursery, there is no way to achieve 1) from a fixture, so this means the test would look like this:

async def test_server_connection():

    async def actual_test(listerner):
        connection = await connect_to_server(listerner)  # step 2)
        ret = await connection.ping()  # step 3)
        assert ret == 'ok'
        nursery.cancel_scope.cancel()  # step 4)

    async with trio.open_nursery() as nursery:
        listerners = nursery.start(trio.serve_tcp, my_server_handle_client, 0)  # step 1)
        nursery.start_soon(actual_test, listerners[0])

This is pretty cumbersome (especially with multiple tests, each one needing to start the server).

A solution to this trouble could be to write a decorator for the test:

async def with_server(fn):

    @wraps(fn)
    async def wrapper(*args, **kwargs):

        async def runtest_then_cancel_scope(listerner):
            await fn(listerner, *args, **kwargs)
            nursery.cancel_scope.cancel()  # step 4)

        async with trio.open_nursery() as nursery:
            listeners = nursery.start(trio.serve_tcp, my_server_handle_client, 0)  # step 1)
            nursery.start_soon(runtest_then_cancel_scope, listerners[0])


@with_server
async def test_server_connection(server):
    connection = await connect_to_server(server)  # step 2)
    ret = await connection.ping()  # step 3)
    assert ret == 'ok'

While the test looks much better, boilerplates are still pretty cumbersome. On the top of that this code is wrong because @wraps will pass all the functions parameters to the parent caller which in turn will consider them as fixtures to find. However the first parameter server is in fact injected by the decorator itself ! The work around is to create a custom wraps function that discard the first argument before doing the decoration... This is getting out of hand pretty quickly !

To solve all this, I'm thinking the user test function could be run into a nursery, this way we could provide a nursery fixture to access it, allowing to easily create fixture responsible to start a server.

@pytest.fixture
async def server(nursery):
    listeners = await nursery.start(trio.serve_tcp, my_server_handle_client, 0)  # step 1)
    return listeners[0]


async def test_server_connection(server):
    connection = await connect_to_server(server) # step 2)
    ret = await connection.ping()  # step 3)
    assert ret == 'ok'

Finally we would call nursery.cancel_scope.cancel() once the teardown of the fixture have been done (which would achieve step 4 automatically).

This seems much more elegant and straightforward, however I'm still unsure if canceling this scope automatically is such a good idea (this could lead to normally infinite loop that would end up hidden).
Another solution would be to take advantage of the async yield fixture to do stop the server during teardown of the fixture:

@pytest.fixture
async def server(nursery):
    with trio.open_cancel_scope():
        listeners = await nursery.start(trio.serve_tcp, handle_client, 0)
        yield listeners[0]

However this doesn't seems to work:

../../.local/share/virtualenvs/pytest-trio-fHSyI99q/lib/python3.6/site-packages/trio/_core/_run.py:1502: in run_impl
    msg = task.coro.send(next_send)
pytest_trio/plugin.py:96: in teardown
    await self._teardown()
pytest_trio/plugin.py:119: in _teardown
    await self.agen.asend(None)
pytest_trio/_tests/test_try.py:14: in server
    yield listeners[0]
/usr/local/lib/python3.6/contextlib.py:89: in __exit__
    next(self.gen)
../../.local/share/virtualenvs/pytest-trio-fHSyI99q/lib/python3.6/site-packages/trio/_core/_run.py:206: in open_cancel_scope
    scope._remove_task(task)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <cancel scope object at 0x7f1ee62c41d0>, task = <Task 'pytest_trio.plugin._setup_async_fixtures_in.<locals>._resolve_and_update_deps' at 0x7f1ee62c4208>

    def _remove_task(self, task):
        with self._might_change_effective_deadline():
>           self._tasks.remove(task)
E           KeyError: <Task 'pytest_trio.plugin._setup_async_fixtures_in.<locals>._resolve_and_update_deps' at 0x7f1ee62c4208>

../../.local/share/virtualenvs/pytest-trio-fHSyI99q/lib/python3.6/site-packages/trio/_core/_run.py:165: KeyError

I'm not sure if this bug is due to a conflict between open_nursery and the open_cancel_scope over the server_tcp coroutine or a more generic bug caused by how an async yield fixture is created inside the setup coroutine but finished consumed into a teardown one...

@njsmith I would be really interested to have your point of view on this
I've created a branch implementing this feature, you can have a look at the test crashing

@njsmith
Copy link
Member

njsmith commented Dec 18, 2017

What do you think of:

@fixture
async def server_fixture():
    async with trio.open_nursery() as nursery:
        await nursery.start(some_server, ...)
        yield
        nursery.cancel_scope.cancel()

?

This has the problem that it violates the "parenting is a full time job" rule, but at this point I think of that as a bug in Trio, rather than a bug in this code :-). I'm currently working on fixing Trio to get rid of the parenting-is-a-full-time-job rule.

In your code, I think you meant to write something like this?

@pytest.fixture
async def server(nursery):
    with trio.open_cancel_scope() as cancel_scope:
        listeners = await nursery.start(trio.serve_tcp, handle_client, 0)
        yield listeners[0]
        cancel_scope.cancel()

Unfortunately this doesn't work -- here the serve_tcp call gets moved into the nursery as soon as it finishes starting up, so the cancel scope doesn't apply to it.

However, the traceback you got is a real problem: open_cancel_scope assumes that you'll execute both the __enter__ and __exit__ from within the same task, and open_nursery is the same. But right now we definitely aren't doing anything to make that happen; in fact we're spawning a separate task to run each of them :-(.

I feel sad about losing the cool recursive/parallel fixture setup, but maybe we need to switch to something simpler, where we instantiate the fixtures serially in the main task, and then clean them up in serial in the opposite order.

@touilleMan
Copy link
Member Author

touilleMan commented Dec 19, 2017

This has the problem that it violates the "parenting is a full time job" rule

Yep, that was the reason why I didn't want to do it this way...

I'm currently working on fixing Trio to get rid of the parenting-is-a-full-time-job rule.

Cool ! do you have a link with more info about this ?

In your code, I think you meant to write something like this?

Yeah I wasn't sure the cancel_scope.cancel() was mandatory when using open_cancel_scope() (I though the cancel was automatic once reached the __exit__)

I feel sad about losing the cool recursive/parallel fixture setup, but maybe we need to switch to something simpler, where we instantiate the fixtures serially in the main task, and then clean them up in serial in the opposite order.

I've created PR #26 that remove the parallel fixture setup/teardown. This way the nursery creation from within the fixture works well. However this is not perfect for error handling coming from the coroutine started within the nursery:

Considering:

async def handle_client(stream):
    while True:
        raise RuntimeError('fooo')
        buff = await stream.receive_some(4)
        await stream.send_all(buff)

We end up with:

E                   trio.TrioInternalError: internal error in trio - please file a bug!

../../.local/share/virtualenvs/pytest-trio-fHSyI99q/lib/python3.6/site-packages/trio/_core/_run.py:1394: TrioInternalError
============================================================================================= 1 failed in 0.11 seconds ==============================================================================================
Exception ignored in: <bound method Nursery.__del__ of <trio._core._run.Nursery object at 0x7fa2091536a0>>
Traceback (most recent call last):
  File "/home/emmanuel/.local/share/virtualenvs/pytest-trio-fHSyI99q/lib/python3.6/site-packages/trio/_core/_run.py", line 522, in __del__
AssertionError: 
Exception ignored in: <bound method AsyncGenerator.__del__ of <async_generator.impl.AsyncGenerator object at 0x7fa209153080>>
Traceback (most recent call last):
  File "/home/emmanuel/.local/share/virtualenvs/pytest-trio-fHSyI99q/lib/python3.6/site-packages/async_generator/impl.py", line 310, in __del__
RuntimeError: partially-exhausted async_generator garbage collected
Exception ignored in: <bound method Nursery.__del__ of <trio._core._run.Nursery object at 0x7fa209133cf8>>
Traceback (most recent call last):
  File "/home/emmanuel/.local/share/virtualenvs/pytest-trio-fHSyI99q/lib/python3.6/site-packages/trio/_core/_run.py", line 522, in __del__
AssertionError: 
Exception ignored in: <async_generator object server at 0x7fa2095edc78>
Traceback (most recent call last):
  File "/home/emmanuel/projects/pytest-trio/pytest_trio/_tests/test_nursery_fixture.py", line 17, in server
  File "/home/emmanuel/.local/share/virtualenvs/pytest-trio-fHSyI99q/lib/python3.6/site-packages/trio/_util.py", line 120, in __aexit__
  File "/home/emmanuel/.local/share/virtualenvs/pytest-trio-fHSyI99q/lib/python3.6/site-packages/async_generator/impl.py", line 264, in athrow
  File "/home/emmanuel/.local/share/virtualenvs/pytest-trio-fHSyI99q/lib/python3.6/site-packages/async_generator/impl.py", line 277, in _do_it
  File "/home/emmanuel/.local/share/virtualenvs/pytest-trio-fHSyI99q/lib/python3.6/site-packages/async_generator/impl.py", line 187, in __next__
  File "/home/emmanuel/.local/share/virtualenvs/pytest-trio-fHSyI99q/lib/python3.6/site-packages/async_generator/impl.py", line 199, in _invoke
  File "/home/emmanuel/.local/share/virtualenvs/pytest-trio-fHSyI99q/lib/python3.6/site-packages/trio/_core/_run.py", line 320, in open_nursery
  File "/usr/local/lib/python3.6/contextlib.py", line 100, in __exit__
  File "/home/emmanuel/.local/share/virtualenvs/pytest-trio-fHSyI99q/lib/python3.6/site-packages/trio/_core/_run.py", line 206, in open_cancel_scope
  File "/home/emmanuel/.local/share/virtualenvs/pytest-trio-fHSyI99q/lib/python3.6/site-packages/trio/_core/_run.py", line 165, in _remove_task
KeyError: <Task 'pytest_trio.plugin._trio_test_runner_factory.<locals>._bootstrap_fixture_and_run_test' at 0x7fa209133a20>

So I'm still hoping for a better way to do that...

@njsmith
Copy link
Member

njsmith commented Dec 20, 2017

For parenting no longer being a full time job: python-trio/trio#375

And..... crud. You're right. I totally forgot that @fixture pretends to be like @contextmanager in most ways, except that it doesn't give access to exceptions.

So... ugh. I know why pytest did it this way: their conceptual model is that when they setup/teardown fixtures, that can raise an exception and they can handle it and attribute it to that fixture, and when they run a test, that can raise an exception and they can handle it and attribute it to that test. But that model breaks down in the face of concurrency: if a fixture is running code in the background, then it can fail at any moment, including while a test is running, and in that case the thing to do is to unwind out of that test -- marking it as incomplete or errored or something -- and give up on any other tests that use that fixture, and then report the error in the fixture. That's just the facts of life. The way Trio encodes those facts into Python code is that if a fixture's background task fails while a test is running, it injects a Cancelled exception into the test, which is supposed to unwind all the back out through pytest's machinery until it reaches the fixture. And if pytest were built to support async/await and Trio natively, that's how it would work, and it would make perfect sense for fixtures to act like context managers – you'd end up with an effective call stack like: trio.run → pytest fixture management code → fixture → pytest test management code → test, and exceptions would bubble out until they reached the point that could handle them: most would be caught by the test management code, but Cancelled would keep going.

That's not how pytest works though.

It could be how our fixtures work, I guess – we could make @trio_fixture work like a context manager, but only allow it to see Cancelled exceptions. (Or MultiErrors containing nothing but Cancelled exceptions.) I'm worried though that that would be dangerously confusing, since people are used to fixtures like

@fixture
def tempfile():
    name = create_tempfile()
    yield name
    os.unlink(name)

and that'd be subtly broken if you forgot and used the same pattern for a @trio_fixture, because you'd have to instead write:

@trio_fixture
def tempfile():
    name = create_tempfile()
    try:
        yield name
    finally:
        os.unlink(name)

In a perfect world with infinite resources I guess we could reimplement pytest to work the way we want, but in this world I think maybe we better stick to the pytest way of doing things, even if it's kinda broken... the alternative just seems too error prone.

So if we can't do the "obvious" thing, then I guess this suggests that we should do two things:

  • Forbid fixtures from yielding inside cancel scopes (which includes nurseries). There's some discussion of this in [discussion] What to do about cancel scopes / nurseries inside generators? trio#264, but basically the tl;dr is that we probably can't solve this in general without first adding features to the interpreter, so this would need some pytest-trio specific hack to catch. (I think implementing this will need a bit of help from trio proper -- in particular some way to ask trio whether a new cancel scope has been opened. Maybe a trio.hazmat.cancel_scope_depth function that we could call before and after initializing the fixture to check if it changed?)

  • Provide a special magical fixture_nursery fixture which fixtures can use to spawn background tasks, and which automatically gets cancelled at some point – after fixture cleanup, I guess.

Does... that make sense? I feel like I've been completely wrong about this stuff several times now so I'm wondering what I missed this time :-)

@touilleMan
Copy link
Member Author

touilleMan commented Dec 20, 2017

For parenting no longer being a full time job: python-trio/trio#375

This is soooo cool !

This allow simple coroutine creation from within async with statements:

def with_server(fn):

    async def wrapper(*args, **kwargs):

        async def run_coroutine_then_cancel_nursery(nursery, coroutine):
            await coroutine
            nursery.cancel_scope.cancel()

        async with trio.open_nursery() as nursery:
            server_address = await nursery.start(run_server)
            nursery.start_soon(
                run_coroutine_then_cancel_nursery,
                nursery,
                fn(server_address, *args, **kwargs)
            )

    return wrapper


async def test_connect_to_server():
    @with_server
    async def tester(address):
        conn = await open_connection(address)
        res = await conn.ping()
        assert res == 'ok'

    await tester()

becomes:

@trio._util.acontextmanager
async def start_server():
    async with trio.open_nursery() as nursery:
        server_address = await nursery.start(run_server)
        yield server_address
        nursery.cancel_scope.cancel()


async def test_connect_to_server():
    async with start_server() as server_address:
        conn = await open_connection(server_address)
        res = await conn.ping()
        assert res == 'ok'

Obviously only drawback is the second solution makes use of trio._util.acontextmanager which is not exposed by trio. This is more a trouble of Python itself though given it doesn't provide (yet ?) a simple way to do async context manager like contextlib.contextmanager.
Considering this usecase, maybe it would be useful to move this acontextmanager function to it own repo (or make it part of async_generator package) ?

@njsmith
Copy link
Member

njsmith commented Dec 20, 2017

This allow simple coroutine creation from within async with statements:

Yes, exactly :-)

This is more a trouble of Python itself though given it doesn't provide (yet ?) a simple way to do async context manager like contextlib.contextmanager.

Yeah, this is an ongoing irritation. I don't want to expose it in the trio namespace, and I keep hoping that someone else will do it, but that hasn't happened yet... Python 3.7 will have it (contextlib.asynccontextmanager), but that doesn't do much good for us now, or for lots of people for quite some time. Normally contextlib2 would take care of this (it's the backported version of contextlib), but there's no sign of motion there either (see jazzband/contextlib2#12).

Given all this, I think I'd be fine with moving it into async_generator (and renaming to asynccontextmanager to match the 3.7 version). It's even kind of nice in that if you're on 3.5 and want to use this, well, you need async_generator anyway and hey now it's right there. And if you're on 3.6, then you're pulling in async_generator, at which point you might as well be 3.5-compatible too.

Are you interested in putting together a PR for this?

(It might even make sense to move async_generator into the python-trio org while we're at it – after all, it's all in the same general space of making async more usable...)

@touilleMan
Copy link
Member Author

I may have a solution for our trouble #27 ;-)

The idea is to consider each fixture as an async context manager, this way setup and teardown steps are both done from the same coroutine.

On top of that, it allow us to catch an exception raised from the user test function, leave the context managers of the fixture like everything is fine, then re-raise the exception.
This way fixtures teardown work just as expected (minus the fact that an exception from the fixture setup/teardown is still seen by pytest as a test FAILURE where it should be an ERROR).

@njsmith
Copy link
Member

njsmith commented Dec 20, 2017 via email

@njsmith
Copy link
Member

njsmith commented Dec 21, 2017

Hmm, looking at this again, and your PR, I guess I see how it could work after all. It's making my brain hurt a bit though :-).

Basically the way it's written, if a background task in a fixture crashes, then it causes the actual test to raise Cancelled, and then ... we absorb that exception. (As currently written I'm not sure it handles KeyboardInterrupt properly, but we can check.) In general in Trio this is totally wrong, you can't throw away Cancelled exceptions, but in this case we know we're about to exit so...... I guess maybe we can get away with it? In particular we know that any Cancelled exceptions must be triggered by cancel scopes inside the fixtures, which we're in the middle of closing anyway, because we know that we just called trio.run ourselves. So we know that the test has finished with some kind of error status, and that the Cancelled exceptions were going to be discarded while we went through our fixtures...

I guess there are two places where not threading the exceptions through properly causes some difference in effects:

  • Normally, it makes a difference whether you exit a nursery via an exception or not, because the exception triggers cancellation of everything else in the nursery. But in this case any fixture that has a nursery had better be cancelling everything in it anyway, because that's what it means to tear down a fixture, so maybe the difference is not a big deal in practice?

  • Normally, it a cancel scope records whether it caught an exception (in its .cancelled_caught attribute), which is used to implement things like fail_after. In this case I can't see any case where you'd actually care about the accuracy of this attribute though...

This all pretty confusing though. And one of the things I realized here: in practice, when you have two fixtures and they each make their own nursery, they end up coupled anyway, because if the fixture that happens to arbitrarily be treated as the "outer" nursery crashes, then the other nursery is automatically cancelled. (Plus of course they're also coupled by the fact that they're both being set up for the same test, and both will torn down at the same time regardless.)

So I'm still wondering if it might not end up being simpler all around to stick with a nursery_fixture fixture that all the fixtures are supposed to use. In practice this probably simplifies most fixtures too, because it means you don't have to keep reimplementing the cancel-on-teardown logic, i.e. compare:

async def server_fixture():
    async with trio.open_nursery() as nursery:
        nursery.start_soon(whatever)
        yield
        nursery.cancel_scope.cancel()

versus

def server_fixture(nursery_fixture):
    nursery_fixture.start_soon(whatever)

I'm going to sleep on it :-). Either way I'm glad we're having this discussion, this is super confusing and digging into it like this is the only way to bring some clarity and hopefully get the best final result...

@touilleMan
Copy link
Member Author

Basically the way it's written, if a background task in a fixture crashes, then it causes the actual test to raise Cancelled, and then ... we absorb that exception.

I guess a less violent way of achieving this would be to wrap the yield of the fixture with a try/finally block to have the teardown part executed no matter what.

So I'm still wondering if it might not end up being simpler all around to stick with a nursery_fixture fixture that all the fixtures are supposed to use.

The trouble I see with the nursery fixture is you don't have a simple way cancel the coroutine you submitted:

@pytest.fixture
async def my_fixture():
    async with trio.open_nursery() as nursery:
        listeners = await nursery.start(await trio.serve_tcp, handle_client, 0)
        yield listeners
        nursery.cancel_scope.cancel()

becomes:

@pytest.fixture
async def my_fixture(nursery):
    with trio.open_cancel_scope() as cancel_scope:
        listeners = await nursery.start(await trio.serve_tcp, handle_client, 0)
        yield listeners
        cancel_scope.cancel()

So in the end we didn't get any benefit from the nursery fixture. Worst, having to handle our own cancel scope is something this not as usual as directly using a nursery so I guess it makes things more complicated.
Obviously we could instead automatically call the nursery.cancel_scope.cancel() once the test is over. But this would break the explicit is better than implicit zen of Python, so not sure it is worth it.

@njsmith
Copy link
Member

njsmith commented Dec 23, 2017

I guess a less violent way of achieving this would be to wrap the yield of the fixture with a try/finally block to have the teardown part executed no matter what.

Right -- this is what I was thinking of up above when I mentioned "we could make @trio_fixture work like a context manager ...". The problem is the what I said there though – that's just not how pytest fixtures work normally, so there's a lot of potential for confusion. And unfortunately, the problem is pretty subtle: if someone accidentally writes their fixture like a normal pytest fixture, then it'll seem to work correctly most of the time... until something goes wrong and their cleanup code fails to run. Maybe it'd be worth it if doing this gave users some major advantage over the other approaches, but I'm not really seeing it.

So in the end we didn't get any benefit from the nursery fixture

Another wrinkle: your example with open_cancel_scope doesn't actually work :-). Once the task is started in the nursery, it moves "out of" the place where it was created and "in to" the nursery, and in the process it moves out of the cancel scope you made.

Obviously we could instead automatically call the nursery.cancel_scope.cancel() once the test is over. But this would break the explicit is better than implicit zen of Python, so not sure it is worth it.

Well, but practicality beats purity :-). I think that's why they call it the "zen" of python -- all the lines contradict each other :-).

More seriously – do I think it would be OK for a fixture nursery be automatically cancelled once the test is over and all the fixtures have been cleaned up. For one thing, what else are you going to do once you reach that point? The test is done, it's time to move on. And for another, it's just super handy – and for testing code, convenience is relatively more important compared to regular code. (See also: pytest's use of code introspection for test autodiscovery, which would be horrifying in a regular program but is excellent for testing.) In particular, pretty much every single fixture would otherwise have to re-implement the final cancellation itself, like:

@pytest.fixture
async def my_fixture():
    async with trio.open_nursery() as nursery:
        listeners = await nursery.start(trio.serve_tcp, handle_client, 0)
        yield listeners
        nursery.cancel_scope.cancel()

is pretty wordy, but if there's a fixture nursery that auto-cancels itself, then it becomes just

@pytest.fixture
async def my_fixture(fixture_nursery):
    return await nursery.start(trio.serve_tcp, handle_client, 0)

Also, having a special named nursery that's documented to always have this behavior is actually fairly explicit, IMO. We could even name it something like autocancel_nursery if we want...

@touilleMan
Copy link
Member Author

I've added the nursery fixture with automatic cancel in PR #28
This gave us the best of both world: user can choose to use the nursery fixture to go simble, or use the async context manager fixture to create an use it own nursery.

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 a pull request may close this issue.

2 participants