refactor: update request-id recipe to use contextvars #2382
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2382 +/- ##
===========================================
+ Coverage 99.92% 100.00% +0.07%
===========================================
Files 64 64
Lines 7726 7726
Branches 1071 1071
===========================================
+ Hits 7720 7726 +6
+ Misses 5 0 -5
+ Partials 1 0 -1 ☔ View full report in Codecov by Sentry. |
vytas7
changed the title
There was a problem hiding this comment.
Thanks for this improvement @EricGoulart, your code looks good. 👍
Could you also try adding a couple of tests to tests/test_recipes.py? (Check out other recipe tests for inspiration.)
Thank you for the feedback! I'll go ahead and add a couple of tests to tests/test_recipes.py as suggested. |
|
Btw I don't know if you are participating in Hacktoberfest, but this is already good enough to qualify, I'll add the label just in case. |
tests/test_recipes.py
Outdated
| import pytest | ||
|
|
||
| from examples.recipes.request_id_middleware import RequestIDMiddleware |
There was a problem hiding this comment.
It's not a good idea to import this way, it may not work correctly when running pytest from another directory, for instance.
Let's import directly from the Python file, as it is done in other tests in this file.
tests/test_recipes.py
Outdated
| return response.json['request_id'] | ||
|
|
||
| loop = asyncio.get_event_loop() | ||
| request_id1, request_id2 = loop.run_until_complete( |
There was a problem hiding this comment.
These won't run asynchronously anyway, there is no real benefit vs just running make_request() twice.
(If we were testing an async app, we could achieve this with ASGIConductor.)
|
Sure, I’ll check into it and make sure the import errors are resolved. |
|
Hi @EricGoulart, did you intend to close this PR? |
|
Aha, I see you opened another one for the same changeset. We can continue the review process on that one instead. |
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.