-
Notifications
You must be signed in to change notification settings - Fork 139
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
Add support for wait_readable() and wait_writable() on ProactorEventLoop #831
Conversation
for more information, see https://pre-commit.ci
Also removed some dead code.
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 pretty great overall, this is going to be a killer feature.
Just some nits, and I think you need to tweak the socketpair impl
src/anyio/_backends/_asyncio.py
Outdated
event = asyncio.Event() | ||
try: | ||
loop.add_writer(obj, event.set) | ||
remove_writer = loop.remove_writer |
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.
Nit: I think this should be in the except's else clause
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 thought someone might pick up on that...
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.
Fixed.
def __init__(self) -> None: | ||
self._thread = threading.Thread(target=self.run, name="AnyIO socket selector") | ||
self._selector = DefaultSelector() | ||
self._send, self._receive = socket.socketpair() |
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 think both sockets should be non-blocking, and you should ignore BlockingIOError on the send side
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.
Done.
while not self._closed: | ||
for key, events in self._selector.select(): | ||
if key.fileobj is self._receive: | ||
self._receive.recv(10240) |
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 think this needs to be a loop with a smaller buffer size:
https://github.com/python/cpython/blob/3.13/Lib/asyncio/selector_events.py#L130
Also handing Blocking and Interrupted errors
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.
Possibly both of these should be recv_into?
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 get BlockingIOError
, but why InterruptedError
too? System calls are automatically retried by Python whenever they're interrupted.
pass # the loop was already closed | ||
|
||
self._selector.unregister(self._receive) | ||
self._receive.close() |
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 think this should be closed using with
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 also think you need to close the selector https://docs.python.org/3/library/selectors.html#selectors.BaseSelector.close
Ah you probably want to close it in the join
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 closing the selector and the socket in stop()
now.
I messed up somewhere along the way and now it gets stuck in a busy-wait loop somewhere. |
src/anyio/_core/_sockets.py
Outdated
This does **NOT** work on Windows when using the asyncio backend with a proactor | ||
event loop (default on py3.8+). | ||
On backends where this functionality is not natively provided (asyncio | ||
``ProactorEventLoop`` on Windows) Additionally, on asyncio, this functionality is |
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.
``ProactorEventLoop`` on Windows) Additionally, on asyncio, this functionality is | |
``ProactorEventLoop`` on Windows), this functionality is |
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.
Woops. I pushed a better wording just now.
Changes
This enables asyncio on Windows to call
wait_readable()
andwait_writable()
by using a selector on a separate thread.Closes #820.
Checklist
If this is a user-facing code change, like a bugfix or a new feature, please ensure that
you've fulfilled the following conditions (where applicable):
tests/
) added which would fail without your patchdocs/
, in case of behavior changes or newfeatures)
docs/versionhistory.rst
).If this is a trivial change, like a typo fix or a code reformatting, then you can ignore
these instructions.
Updating the changelog
If there are no entries after the last release, use
**UNRELEASED**
as the version.If, say, your patch fixes issue #123, the entry should look like this:
* Fix big bad boo-boo in task groups (#123 <https://github.com/agronholm/anyio/issues/123>_; PR by @yourgithubaccount)
If there's no issue linked, just link to your pull request instead by updating the
changelog after you've created the PR.