Add nursery fixture #25
Comments
|
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? Unfortunately this doesn't work -- here the However, the traceback you got is a real problem: 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. |
Yep, that was the reason why I didn't want to do it this way...
Cool ! do you have a link with more info about this ?
Yeah I wasn't sure the
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: So I'm still hoping for a better way to do that... |
|
For parenting no longer being a full time job: python-trio/trio#375 And..... crud. You're right. I totally forgot that 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 That's not how pytest works though. It could be how our fixtures work, I guess – we could make @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
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:
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 :-) |
This is soooo cool ! This allow simple coroutine creation from within 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 |
Yes, exactly :-)
Yeah, this is an ongoing irritation. I don't want to expose it in the Given all this, I think I'd be fine with moving it into Are you interested in putting together a PR for this? (It might even make sense to move |
|
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. |
|
Unfortunately there's a confusing detail here that I should maybe have
pointed out more clearly: the cancel scope and nursery fixtures actually
*need* to receive any exceptions raised while they're active. For cancel
scopes, this is so they can look at passing Cancelled exceptions and decide
to either catch them or not. For nurseries, it's because every nursery has
an implicit cancel scope, so the same logic applies. Given this, I think
we're stuck: it means that if a fixture contains a nursery, we *must* throw
in any exceptions we get, but if a fixture is written in the usual pytest
style, then we *must not* throw in any exceptions because that will prevent
teardown. (This is the point I was making with the 'tempfile' example
above.) Do you see any way around this?
…On Dec 20, 2017 5:23 AM, "Emmanuel Leblond" ***@***.***> wrote:
I may have a solution for our trouble #27
<#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).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#25 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAlOaO8rATp4fsI5Z5MxU5anVniAmpC-ks5tCQpSgaJpZM4RGFMi>
.
|
|
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 I guess there are two places where not threading the exceptions through properly causes some difference in effects:
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 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... |
I guess a less violent way of achieving this would be to wrap the
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. |
Right -- this is what I was thinking of up above when I mentioned "we could make
Another wrinkle: your example with
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... |
|
I've added the |
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:
Without nursery, there is no way to achieve 1) from a fixture, so this means the test would look like this:
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:
While the test looks much better, boilerplates are still pretty cumbersome. On the top of that this code is wrong because
@wrapswill pass all the functions parameters to the parent caller which in turn will consider them as fixtures to find. However the first parameterserveris in fact injected by the decorator itself ! The work around is to create a customwrapsfunction 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
nurseryfixture to access it, allowing to easily create fixture responsible to start a server.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:
However this doesn't seems to work:
I'm not sure if this bug is due to a conflict between
open_nurseryand theopen_cancel_scopeover theserver_tcpcoroutine or a more generic bug caused by how an async yield fixture is created inside thesetupcoroutine but finished consumed into ateardownone...@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
The text was updated successfully, but these errors were encountered: