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

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

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

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

vytas7 opened this issue Aug 8, 2024 · 4 comments · May be fixed by #2382
Labels
documentation good first issue Comment on this issue if you'd like to volunteer to work on this. Thanks! maintenance
Milestone
Triaged (Non-Brea...

Comments

@vytas7
Copy link
Member

vytas7 commented Aug 8, 2024 •
edited
Loading

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 •
edited
Loading

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

@CaselIT CaselIT mentioned this issue Aug 9, 2024
Merged
@vytas7
Copy link
Member Author

vytas7 commented Aug 10, 2024 •
edited
Loading

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
@EricGoulart
refactor: request-id to use contextvars
2727fef 
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 EricGoulart linked a pull request Oct 20, 2024 that will close this issue
Open
EricGoulart added a commit to EricGoulart/falcon that referenced this issue Oct 29, 2024
@EricGoulart
test(request_id): add tests for request id middleware context handling
9c0f151 
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
@EricGoulart
test(request_id): add tests for request id middleware context handling
54b83b6 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
documentation good first issue Comment on this issue if you'd like to volunteer to work on this. Thanks! maintenance
Projects
None yet
Milestone
Triaged (Non-Breaking Changes)
Development

Successfully merging a pull request may close this issue.

refactor: update request-id recipe to use contextvars EricGoulart/falcon
3 participants
@vytas7 @CaselIT @EricGoulart