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

Preserve contextvars context when handle httpclient response #3340

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tkukushkin
Copy link
Contributor

Hello! We use contextvars and write some of them to JSON logs. Sometimes we see error logs Exception after Future was cancelled. But from the log's context we couldn't find the place where it occured, only that it was related to tornado httpclient. After debugging, we noticed, that this log might be written with context of other tasks that also make HTTP requests. This PR is addressed to fix this issue.

I'm not sure if any tests are needed here, but I can try to add if you think they are.

@bdarnell
Copy link
Member

Yes, this should have a test to make sure it doesn't get broken in the future. Something like ContextVarsTest in gen_test.py.

I'm a little surprised this is necessary - I had thought that the use of contextvars in the asyncio event loop covered what we needed. I wonder if there are any other callback-oriented interfaces we need to update in tornado.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants