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.

Sign up for GitHub

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

Jump to bottom

ContextVar support? #3406

Closed
Reskov opened this issue Nov 23, 2018 · 5 comments · Fixed by #3442
Closed

ContextVar support? #3406

Reskov opened this issue Nov 23, 2018 · 5 comments · Fixed by #3442
Labels
enhancement outdated

Comments

@Reskov
Copy link

Reskov commented Nov 23, 2018

Long story short

I am trying to check ContextVar integration with aiohttp. https://docs.python.org/3/library/contextvars.html#asyncio-support
ContextVar seems not deals well with aiohttp server... Am I doing something wrong?

Expected behaviour

ContextVar should be different for each request.

Actual behaviour

ContextVar has the same value between requests, but seems to be cleared on failure.

Steps to reproduce

server.py

from contextvars import ContextVar
from aiohttp import web

ctx = ContextVar("test")

async def handle(request):
    try:
        ctx.get()
        assert False, "Shouldn't ever be called. ContextVar has value"
    except LookupError:
        pass

    ctx.set("Test")

    return web.Response(text="Hello, " + ctx.get())

app = web.Application()
app.add_routes([web.get('/', handle)])

web.run_app(app, port=8000)

Other process client.py

Client is working fine, just for example

import aiohttp
import asyncio

async def main():
    async with aiohttp.ClientSession() as session:
        while True:
            async with session.get("http://127.0.0.1:8000/") as response:
                html = await response.text()

            print(html)

asyncio.run(main())

Response

Server is constantly being failed on each even request.

Hello, Test
<html><head><title>500 Internal Server Error</title></head><body><h1>500 Internal Server Error</h1>Server got itself in trouble</body></html>
Hello, Test
<html><head><title>500 Internal Server Error</title></head><body><h1>500 Internal Server Error</h1>Server got itself in trouble</body></html>
Hello, Test
...

Extra example

I tried almost the same for sanic, and seems that it works fine.

from contextvars import ContextVar

from sanic import Sanic
from sanic.response import text

app = Sanic()

ctx = ContextVar("test")


@app.route('/')
async def test(request):
    try:
        ctx.get()
        assert False, "Shouldn't ever be called. ContextVar has value"
    except LookupError:
        pass

    ctx.set("Test")

    return text('hello, ' + ctx.get())
app.run(host='0.0.0.0', port=8000)

Response

hello, Test
hello, Test
hello, Test
hello, Test
hello, Test
hello, Test
hello, Test

Your environment

macOs Mojave
Python 3.7.0 (default, Jun 29 2018, 20:13:13) \n[Clang 9.1.0 (clang-902.0.39.2)]
aiohttp 3.4.4, also tried 3.5.0a1 from master branch
@aio-libs-bot
Copy link

GitMate.io thinks the contributor most likely able to help you is @asvetlov.

Possibly related issues are #667 (Please support multi language url), #105 (chardet support), #33 (HTTPS Support), #3084 (tracing support enhancements), and #6 (HTTP pipelining support improvements).

@asvetlov asvetlov added enhancement and removed bug labels Nov 23, 2018
@asvetlov
Copy link
Member

Good point!
Both aiohttp and sanic doesn't support context vars explicitly but aiohttp creates a task for response handling once per a connection. Sanic IIRC creates a task per request.

aiohttp can be modified to create a new contextvar execution context for each web-handler call easily (will work only on Python 3.7 obviously).

We need a champion for the issue.

@Reskov
Copy link
Author

Reskov commented Nov 23, 2018

For now, quick workaround is:

@middleware
def fix_middleware(request, handler):
    return asyncio.create_task(handler(request))

app = web.Application(middlewares=[fix_middleware])

@asvetlov
Copy link
Member

@Reskov thanks for the hint.
Maybe it will be useful for somebody.

asvetlov added a commit that referenced this issue Dec 8, 2018
@asvetlov
Create a task per request handling.
b172008 
Fixes #3406

A separate task for every request doesn't produce any performance degradation on benchmarks.

Tests update is needed to wait not only a task started by connection_made() but also a nexted task.
@asvetlov asvetlov mentioned this issue Dec 8, 2018
Merged
@lock
Copy link

lock bot commented Dec 9, 2019

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Dec 9, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Dec 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
enhancement outdated
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Create a task per request handling. aio-libs/aiohttp
3 participants
@asvetlov @Reskov @aio-libs-bot