refactor: update request-id recipe to use contextvars #2404
Conversation
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
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
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
|
I wanted to apologize for the confusion with my pull request. This is my first time contributing to a public repository, and I made a mistake with the process. I had previously opened a PR with the changes, but I accidentally closed it and am unable to reopen it. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2404 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 64 64
Lines 7728 7728
Branches 1071 1071
=========================================
Hits 7728 7728 ☔ View full report in Codecov by Sentry. |
|
|
||
| response = client.simulate_get('/test') | ||
| assert 'X-Request-ID' in response.headers | ||
| assert response.headers['X-Request-ID'] == response.json['request_id'] |
There was a problem hiding this comment.
could also test that's the same as the value returned in the body
There was a problem hiding this comment.
Thanks, this looks good now at first glance. 👍
However, many CI gates are still failing, could you please take a look?
(To save some time on review and CI rounds, it may be more efficient to run tox locally first on your side, and make sure everything is green.)
Sorry for the above comment, all these failures were caused by a new version of the websockets test dependency, I have already fixed that. So all the checks are ostensibly green now.
However, there test issue that I pointed to before still remains. We don't run these tests upon every PR, but we do for master merge.
If you have access to Docker, could you pip install cibuildwheel, and run:
CIBW_BUILD=cp312-manylinux_x86_64 cibuildwheel
⬇️
<...>
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
# middleware.py
from uuid import uuid4
> from .request_id_context import ctx
E ModuleNotFoundError: No module named 'examples'
/project/examples/recipes/request_id_middleware.py:5: ModuleNotFoundError
=========================== short test summary info ============================
ERROR ../../../project/tests/test_recipes.py::TestRequestIDContext::test_request_id_isolated
ERROR ../../../project/tests/test_recipes.py::TestRequestIDContext::test_request_id_persistence
ERROR ../../../project/tests/test_recipes.py::TestRequestIDContext::test_request_id_in_response_header
================= 3570 passed, 463 skipped, 3 errors in 31.92s =================
⬆️ I'm still getting new errors specifically from your tests, so we must address them in any case.
| @@ -10,14 +10,14 @@ to every log entry. | |||
|
|
|||
| If you wish to trace each request throughout your application, including | |||
| from within components that are deeply nested or otherwise live outside of the | |||
| normal request context, you can use a `thread-local`_ context object to store | |||
| normal request context, you can use a `contextvars`_ object to store | |||
There was a problem hiding this comment.
We could simply replace this with an Intersphinx link to the contextvars module, but this applies to the current version too, so not a requirement for merging this.
Summary of Changes
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.
Related Issues
Closes #2260.