aio-libs / pytest-aiohttp

pytest plugin for aiohttp support
Apache License 2.0
134 stars 24 forks source link

can't use context vars in shutdown #18

Closed ojii closed 2 years ago

ojii commented 2 years ago

The following test passes but then errors in the finalizers when it tries to run the app shutdown, as the context is no longer available:

from contextvars import ContextVar

import pytest
from aiohttp import web

CV = ContextVar("CV")

async def startup(_app):
    CV.set(1)

async def shutdown(app):
    assert CV.get() == 1

async def handler(request):
    return web.Response(body=str(CV.get()))

@pytest.fixture
def app():
    app = web.Application()
    app.on_startup.append(startup)
    app.on_shutdown.append(shutdown)
    app.add_routes([web.get("/", handler)])
    return app

@pytest.fixture
def client(app, loop, aiohttp_client):
    return loop.run_until_complete(aiohttp_client(app))

async def test_get(client):
    response = await client.get("/")
    assert await response.content.read() == b"1"

Error:

====================================================================================================================== ERRORS =======================================================================================================================
_______________________________________________________________________________________________________ ERROR at teardown of test_get[pyloop] _______________________________________________________________________________________________________

app = <Application 0x104642b60>

    async def shutdown(app):
>       assert CV.get() == 1
E       LookupError: <ContextVar name='CV' at 0x103b27e20>

test_aiohttp_pytest_cv.py:14: LookupError
--------------------------------------------------------------------------------------------------------------- Captured log teardown ---------------------------------------------------------------------------------------------------------------
ERROR    asyncio:base_events.py:1729 Task was destroyed but it is pending!
task: <Task pending name='Task-6' coro=<RequestHandler.start() done, defined at .../lib/python3.10/site-packages/aiohttp/web_protocol.py:464> wait_for=<Future cancelled>>
============================================================================================================== short test summary info ==============================================================================================================
ERROR test_aiohttp_pytest_cv.py::test_get[pyloop] - LookupError: <ContextVar name='CV' at 0x103b27e20>
============================================================================================================ 1 passed, 1 error in 0.03s =============================================================================================================
Dreamsorcerer commented 2 years ago

Personally, I find ContextVar to be a bad idea, and it's not clear when a context is created/removed. The purpose of ContexVar appears to be more related to have a context for each request or similar, I think using them as an app-wide global will only lead to problems (problems I've recently encountered with another library that uses them like this).

Dreamsorcerer commented 2 years ago

Having said that, it does appear to be documented behaviour: https://docs.aiohttp.org/en/stable/web_advanced.html#contextvars-support So, I'm wondering if the bug might even be in aiohttp.

webknjaz commented 2 years ago

I believe that @asvetlov mentioned that there's something that needs to be improved wrt case (long ago, in private chats). But I don't remember what exactly and I ended up not going down the rabbit hole myself back then. I seem to recall some connection to the cleanup contexts but maybe I'm misremembering.

asvetlov commented 2 years ago

Should be done as a part of pytest-asyncio plugin. Stay tuned.

asvetlov commented 2 years ago

@Dreamsorcerer contextvars were initially created to solve decimal localcontext problem when it is used in async code.

Generally speaking, they are as bad as thread-local variables, not better and not worse. I'm trying to avoid contextvars usage in architecture design, but they are useful for tooling like logging, distributed tracing, etc.