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

contextvars setup when mixing sync and async pytest fixtures in async test #614

Open
2 tasks done
heqile opened this issue Sep 8, 2023 · 13 comments
Open
2 tasks done
Labels
bug Something isn't working

Comments

@heqile
Copy link

heqile commented Sep 8, 2023

Things to check first

  • I have searched the existing issues and didn't find my bug already reported there

  • I have checked that my bug is still present in the latest release

AnyIO version

4.0.0

Python version

3.8

What happened?

In the tests, we need to set the ContextVar to some special values by using pytest fixtures. After updating to v4.0.0, some of our tests broke, as if the sync fixture who setup ContextVar does not work.

I do notice the changes in v4.0.0:

The asynchronous test runner runs all async fixtures and tests in the same task, so context variables set in async fixtures or tests, within an async test runner, will affect other async fixtures and tests within the same runner. However, these context variables are not carried over to synchronous tests and fixtures, or to other async test runners.

In fact, when setting a context variable in a synchronous fixture, the result of setting that context variable is not visible in an async test when using anyio's test runner if an async fixture has been run before the synchronous fixture. (see the code below to reproduce).

But the case that a ContextVar setup in sync fixture and runs after certain async fixture should also be a common use case, (especially when you want to re-use the setup fixutre in other non-async project). And it does work in v3.7.1, so I'd like to consider it as a bug.

(correct me, if I'm wrong :) )

How can we reproduce the bug?

import pytest
import contextvars

pytestmark = pytest.mark.anyio

fixture_var = contextvars.ContextVar("fixture_var")

@pytest.fixture
def anyio_backend():
    return "asyncio"

@pytest.fixture
async def async_fixture():
    yield

@pytest.fixture
def set_context_var():
   reset_token = fixture_var.set("set")
   yield
   fixture_var.reset(reset_token)

async def test_async_fixture_before(async_fixture: str, set_context_var: str):
    assert "set" == fixture_var.get()  # works in anyio 3.7.2, LookupError in anyio 4.0.0

async def test_async_fixture_after(set_context_var: str, async_fixture: str):
    assert "set" == fixture_var.get()  # works in both anyio 3.7.2 and anyio 4.0.0

Thanks @jhominal for reform the tests

@heqile heqile added the bug Something isn't working label Sep 8, 2023
@agronholm
Copy link
Owner

There are two ways you can fix this:

  1. Require anyio_backend or anyio_backend_name as a fixture in set_context_var()
  2. Make set_context_var() an async fixture

Let me know if that works for you, and then we can think about how to improve the documentation.

@jhominal
Copy link
Contributor

Full disclosure: I work in the same team as @heqile, on the affected projects

Both of the proposed solutions are annoying for us, because the synchronous fixtures that we use to set the context variables are located in a pytest plugin that we wrote, and these fixtures are used in both sync and async projects:

  • requiring anyio_backend/anyio_backend_name as a fixture dependency of set_context_var is annoying because that means that I need to define anyio_backend in any non-async project that uses set_context_var, whereas that was not previously the case (it is doable, and I would probably try and create a fake anyio_backend fixture automatically, but I am worried about side effects, such as synchronous tests running multiple times if anyio_backend is parametrized to run once under trio and once under asyncio);
  • defining set_context_var as an async fixture is a bit worse because the sync projects (naturally) do not use an async test runner;

Unless you tell me that you have already ruled out any fix for this issue in anyio, I will take some time in the next few days to investigate the possibility of a fix in anyio.

@agronholm
Copy link
Owner

I haven't touched the pytest plugin in months, but if you have a fix that doesn't break anything, I'm willing to consider it.

@agronholm
Copy link
Owner

I refreshed my memory a bit and it goes something like this: the pytest plugin (actually the backend's runner implementation) creates a runner task which receives requests through a memory object stream. It stays in the background for as long as there are any async fixtures/tests active. But because it works in a long-running task, synchronous fixtures changing context variables won't affect the task.

If you have an alternative solution which also lets one use session/package/module scoped async fixtures, then sure, I'm interested in hearing that proposal.

@jhominal
Copy link
Contributor

Thank you for the explanations, they were very useful.

I have looked into how the plugin works, and my big idea for a fix would be to run all the synchronous fixtures and tests in the background task as soon as the anyio test runner is in use.

If I were to suggest that behavior change in a PR, would you like me to put that behind a pytest configuration switch?

Also, I would like to ask: is it supported to have a synchronous test that requires an asynchronous fixture?

@agronholm
Copy link
Owner

I'm cautiously optimistic about this plan. Layering the scopes in their specific tasks sounds like a good idea. I don't know how it will translate to code, but I'm looking forward to seeing it.

@jhominal
Copy link
Contributor

jhominal commented Sep 15, 2023

I am going to put back the comment that I previously deleted, because you are replying to it here (I was in the process of opening a new issue for the contextvar isolation, but I guess I have your approval to look into how to do it). Thank you!

The more I think about this issue of contextvars.Context and test execution, the more I believe that there needs to be some kind of context isolation between the tests:

  1. At a minimum (I have some tests that are currently broken in anyio 4 because of context promiscuity):
  • All fixtures continue to run in a single background task (like now);
  • Each test function execution runs in a subtask of the fixtures task, so that each test execution can see the Context that was set up by the fixtures, but not be able to modify that context for other tests;
  1. What I would find ideal:
  • All fixtures of a given scope (session/package/module/class/function) run in a single task;
  • Each lower-level fixture task runs as a subtask of its parent - so that fixtures at a given level can see, but not modify, context setups from their parent scope;
  • Each test function execution runs in a subtask of the function-scoped fixtures task;

Do you think that having isolated contextvars.Context between tests is a worthwhile goal in the anyio pytest runner? In other words, would you be open to a PR to implement either (1) or (2) (potentially behind a configuration option)?

Note: While I would find (2) ideal, I believe that it is easy enough for fixtures to clean up after themselves when they set contextvars, so I do not find it strictly necessary, and something that implements (1) would be enough for my needs.

@jhominal
Copy link
Contributor

jhominal commented Sep 15, 2023

@agronholm: Could you just tell me, is it supported to have a test defined by a synchronous function that depends on a fixture implemented in an async function? (I have not been able to make that work, but maybe I am missing something)

@agronholm
Copy link
Owner

I don't think that has ever worked under any async test runner, including AnyIO's. I also don't know how that could possibly work without involving multithreading (which would introduce its own set of issues).

@agronholm
Copy link
Owner

Just to clarify, what I meant was that async fixtures that need to be actively running would be the biggest problem. But say you have an async fixture that just uses an async API to get some data, that could conceivably work.

@agronholm
Copy link
Owner

Btw @jhominal, I pinged you earlier on the encode/community Gitter room about a flaky Starlette test (test_app_receives_http_disconnect_while_sending_if_discarded) that, according to git, you wrote. That test was hampering AnyIO 4 adoption as coverage checks would not always pass. What venue should I use to discuss that with you?

@jhominal
Copy link
Contributor

I have been quite disconnected from gitter and a lot of other tools for a while, we can discuss it on gitter or I can send you my email address so that we can discuss that over there.

@agronholm
Copy link
Owner

I just sent you a PM on Gitter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants