-
-
Notifications
You must be signed in to change notification settings - Fork 37
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
Modernize sniffio integration #124
Conversation
Use `sniffio.thread_local` to indicate which async library is running, instead of the deprecated `sniffio.current_async_library_cvar`. This allows us to remove a number of workarounds for the contextvar being inherited where it shouldn't be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I don't have too much familiarity with sniffio
/trio-asyncio
so can't comment on the code changes, but I have a few comments on meta-changes.
@@ -56,7 +56,7 @@ jobs: | |||
strategy: | |||
fail-fast: false | |||
matrix: | |||
python: ['pypy-3.7', 'pypy-3.8', 'pypy-3.9', '3.7', '3.8', '3.9', '3.10', '3.11', '3.12-dev', 'pypy-3.8-nightly', 'pypy-3.9-nightly'] | |||
python: ['pypy-3.7', 'pypy-3.8', 'pypy-3.9', '3.7', '3.8', '3.9', '3.10', '3.11', 'pypy-3.8-nightly', 'pypy-3.9-nightly'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
python: ['pypy-3.7', 'pypy-3.8', 'pypy-3.9', '3.7', '3.8', '3.9', '3.10', '3.11', 'pypy-3.8-nightly', 'pypy-3.9-nightly'] | |
python: ['pypy-3.7', 'pypy-3.8', 'pypy-3.9', '3.7', '3.8', '3.9', '3.10', '3.11', '3.12', 'pypy-3.8-nightly', 'pypy-3.9-nightly'] |
Maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to do another PR to drop 3.7 and add 3.12 support to CI and the setup.py trove classifiers. For now just wanted to do the minimal thing to get it to build. The error with 3.12-dev is an Ubuntu packaging thing unrelated to trio-asyncio.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I suggested the change cause it looks like the step that fails only runs if the version ends with -dev
(probably remnants of setup-python
not supporting dev versions?). But totally fair to move it to another PR.
Generally, it might be nice if sniffio had a warning if both methods of setting async library name are set? Not sure and tbh I should just make an issue on that repo, just running this by you first! (it negates possible perf improvements, though it could be behind an |
Trio and trio-asyncio are the only libraries that actually integrate with sniffio, so now that they're both updated I'm not sure it's worth the churn. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you've handled everything I can think of!
Use
sniffio.thread_local
to indicate which async library is running, instead of the deprecatedsniffio.current_async_library_cvar
. This allows us to remove a number of workarounds for the contextvar being inherited where it shouldn't be, and is also necessary to interoperate with Trio 0.23 which uses the thread-local.Fixes #123.