provide sniffio.thread_local.name for coroutine libraries to set#23
provide sniffio.thread_local.name for coroutine libraries to set#23graingert merged 12 commits intopython-trio:masterfrom
Conversation
1154f58 to
8050862
Compare
8050862 to
9bb6ca2
Compare
Codecov Report
@@ Coverage Diff @@
## master #23 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 4 4
Lines 71 87 +16
Branches 5 6 +1
=========================================
+ Hits 71 87 +16
|
docs/source/index.rst
Outdated
| token = current_async_library_cvar.set("my-library's-PyPI-name") | ||
| # Your library's step function | ||
| def step(...): | ||
| old_name, current_async_library_tlocal.name = ( |
There was a problem hiding this comment.
I decided to expose the ThreadLocal completely without guardrails, otherwise twisted would have to count the stack frames used by any abstraction
There was a problem hiding this comment.
Would a context manager be a guardrail that would cause trouble? Or perhaps just a performance difference?
from sniffio import enter_async_library
def step(...):
with enter_async_library("mylib"):
result = coro.send(None)There was a problem hiding this comment.
Can you say a bit more about why twisted would have to count stack frames and how that relates to guardrails?
There was a problem hiding this comment.
https://gitter.im/python-trio/general?at=60479824b5131f4f28021225
Kyle Altendorf @altendky 10:45
@graingert:matrix.org since this is related and we're discussing here in gitter: https://github.com/python-trio/sniffio/pull/23#discussion_r590338307 would providing a context manager be problematic?
graingert @graingert:matrix.org [m] 10:46
it's called every step in every coro framework
Kyle Altendorf @altendky 10:46
right, so performance is the concern?
graingert @graingert:matrix.org [m] 10:46
yeah I think so
Probably worth a comment that we are explicitly encouraging not using a context manager.
There was a problem hiding this comment.
Can you say a bit more about why twisted would have to count stack frames and how that relates to guardrails?
Twisted counts stack frames for various purposes including determining what frame defer.returnValue was called from
|
Abbreviating |
docs/source/index.rst
Outdated
| token = current_async_library_cvar.set("my-library's-PyPI-name") | ||
| # Your library's step function | ||
| def step(...): | ||
| old_name, current_async_library_tlocal.name = ( |
There was a problem hiding this comment.
Would a context manager be a guardrail that would cause trouble? Or perhaps just a performance difference?
from sniffio import enter_async_library
def step(...):
with enter_async_library("mylib"):
result = coro.send(None)| There are libraries that can switch back and forth between different | ||
| async modes within a single call-task – like ``trio_asyncio`` or | ||
| Twisted's asyncio operability. These libraries should make sure to set | ||
| the value back and forth at appropriate points. | ||
|
|
||
| The general rule of thumb: :data:`current_async_library_cvar` should | ||
| be set to X exactly at those moments when ``await X.sleep(...)`` will | ||
| work. | ||
|
|
||
| .. warning:: You shouldn't attempt to read the value of | ||
| ``current_async_library_cvar`` directly – | ||
| :func:`current_async_library` has a little bit more cleverness than | ||
| that. |
There was a problem hiding this comment.
Is it somehow now misleading to remind people to think about these things?
There was a problem hiding this comment.
Hmm, yeah, this text all seems like it's still true, just with the threadlocal instead of the cvar.
There was a problem hiding this comment.
I think these warnings no-longer apply - you just make the switch before calling throw(), send(), or close() on a coroutine.
There was a problem hiding this comment.
There are libraries that can switch back and forth between different
async modes within a single call-task – liketrio_asyncioor
Twisted's asyncio operability.
eg this doesn't apply to twisted because an asyncio.Handle callback can't be called from a generator.send
There was a problem hiding this comment.
Isn't it just a reminder to think about the high level cases while working down in the low level implementation? But sure, there's what, three libraries that implement this... But I guess this discussion would continue in the suggested change below, if at all.
There was a problem hiding this comment.
in the current state of the PR we're advising libraries to read the the value of current_async_library_tlocal directly. And current_async_library_cvar is now undocumented
| There are libraries that can switch back and forth between different | ||
| async modes within a single call-task – like ``trio_asyncio`` or | ||
| Twisted's asyncio operability. These libraries should make sure to set | ||
| the value back and forth at appropriate points. | ||
|
|
||
| The general rule of thumb: :data:`current_async_library_cvar` should | ||
| be set to X exactly at those moments when ``await X.sleep(...)`` will | ||
| work. | ||
|
|
||
| .. warning:: You shouldn't attempt to read the value of | ||
| ``current_async_library_cvar`` directly – | ||
| :func:`current_async_library` has a little bit more cleverness than | ||
| that. |
There was a problem hiding this comment.
Isn't it just a reminder to think about the high level cases while working down in the low level implementation? But sure, there's what, three libraries that implement this... But I guess this discussion would continue in the suggested change below, if at all.
…il we drop the dep on contextvars
Co-authored-by: Kyle Altendorf <sda@fstab.net>
|
@graingert, I think my only remaining points were suggesting adding some comments. If you explicitly decline to add them, I'll move on. One bulk refusal here will suffice. |
Co-authored-by: Kyle Altendorf <sda@fstab.net>
|
@njsmith because this PR is unreleased my https://gist.github.com/graingert/5356401e196e3671abfa5e33c969a73e#file-demo-py-L138 |
No description provided.