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

Update request-id recipe to use contextvars instead of threading.local() #2260

Open
vytas7 opened this issue Aug 8, 2024 · 5 comments · May be fixed by #2404
Open

Update request-id recipe to use contextvars instead of threading.local() #2260

vytas7 opened this issue Aug 8, 2024 · 5 comments · May be fixed by #2404
Labels
documentation good first issue Comment on this issue if you'd like to volunteer to work on this. Thanks! maintenance

Comments

@vytas7
Copy link
Member

vytas7 commented Aug 8, 2024

contextvars is a newer module in the stdlib (3.7+) that is more advanced than theading.local() as it not only understand threads, but also asyncio coroutines, etc.

Update the Request ID Logging recipe to use contextvars.

Edit: also add tests for this recipe.

@vytas7 vytas7 added documentation good first issue Comment on this issue if you'd like to volunteer to work on this. Thanks! needs contributor Comment on this issue if you'd like to volunteer to work on this. Thanks! maintenance labels Aug 8, 2024
@CaselIT
Copy link
Member

CaselIT commented Aug 9, 2024

I don't think there is any need to use threadlocal or contextvar. just use req or resp context.

re-reading all that page it proposes several approaches. so yes, context var is better than threadlocal

@vytas7
Copy link
Member Author

vytas7 commented Aug 10, 2024

Indeed, using global thread-locals is usually not a very bright idea and it leads to a spaghetti code base. But sometimes, particularly when working with a code base migrated from another framework that builds on this paradigm, or when very deep inside nested code, it might be a useful escape hatch.

@EricGoulart
Copy link

Hello, I would like to work on this issue. Please let me know if there is any information that I should know before proceeding. Thank you.

@vytas7
Copy link
Member Author

vytas7 commented Oct 7, 2024

Hi @EricGoulart! We are glad to hear, sure, go ahead!
You can skim through our docs on how to contribute to Falcon.

Regarding the issue itself, I hope the description is clear enough, but just ask here or on Gitter if you run into any problems.
The recipe that needs to be updated is this file: docs/user/recipes/request-id.rst (but it also includes snippets from small Python files).

You can render out the docs with tox -e docs. Then open docs/_build/html/user/recipes/request-id.html in your browser.

@vytas7 vytas7 removed the needs contributor Comment on this issue if you'd like to volunteer to work on this. Thanks! label Oct 7, 2024
EricGoulart added a commit to EricGoulart/falcon that referenced this issue Oct 20, 2024
Migrate the request-id handling from threading.local() to contextvars to improve compatibility
with async coroutines and avoid issues with threading. This change ensures that request-id
is properly tracked in asynchronous environments, providing more robust handling in both
sync and async contexts.
Previously, threading.local() was used, which does not handle coroutines effectively.
By using contextvars, we ensure that the request-id remains consistent across async calls.

Closes falconry#2260
EricGoulart added a commit to EricGoulart/falcon that referenced this issue Oct 29, 2024
Add tests to verify that request_id is unique per request and correctly set in response headers. Tests include checks for isolation in async calls and persistence in synchronous requests.

Related to issue falconry#2260
EricGoulart added a commit to EricGoulart/falcon that referenced this issue Oct 29, 2024
Add tests to verify that request_id is unique per request and correctly set in response headers. Tests include checks for isolation in async calls and persistence in synchronous requests.

Related to issue falconry#2260
@EricGoulart
Copy link

Hello, just wanto to say sorry about that, It`s my first time contribuiting to a public repository and I am making some mistakes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation good first issue Comment on this issue if you'd like to volunteer to work on this. Thanks! maintenance
Projects
None yet
3 participants