-
-
Notifications
You must be signed in to change notification settings - Fork 33.7k
gh-81554: Add add_reader support to ProactorEventLoop #141834
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
base: main
Are you sure you want to change the base?
Conversation
v6.5.1 License: Apache-2.0 Copyright (c) 2025 The Tornado Authors unmodified from original, so we can track changes
via background SelectorThread, imported from tornado
| # SPDX-License-Identifier: PSF-2.0 AND Apache-2.0 | ||
| # SPDX-FileCopyrightText: Copyright (c) 2025 The Tornado Authors |
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.
Should this not be in https://github.com/python/cpython/blob/main/Misc/sbom.spdx.json?
See the devguide.
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.
This is not perfectly as-is, since it is adapted and an excerpt, like Lib/asyncio/events.py and a number of others. That doesn't seem to fit the SBOM requirements, in my understanding? I don't see any of the other similarly adapted files in sbom.spdx.json.
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.
After addressing review, it has diverged more substantially. This comment structure seems to be how adapted/partial content is handled. Is there anything in the dev guide for that? I couldn't find it. The SBOM page doesn't seem to apply or mention this sort of case.
Co-authored-by: Stan Ulbrych <[email protected]>
Lib/asyncio/_selector_thread.py
Outdated
| # introduction of a new hook: https://bugs.python.org/issue41962) | ||
| self._thread = threading.Thread( | ||
| name="Tornado selector", | ||
| daemon=True, |
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.
Would it be possible to avoid daemon thread?
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.
#86128 discusses why this is a daemon thread. I believe we need to call
with loop._select_cond:
loop._closing_selector = True
loop._select_cond.notify()
try:
loop._waker_w.send(b"a")
except BlockingIOError:
passprior to thread.join() for it to wake and exit properly. It looks like I can make it a non-daemon thread if I register the atexit hook with the private threading._register_atexit.
I believe this is only needed to handle process teardown for unclosed event loops. If we can strictly guarantee that EventLoop.close() is called before non-daemon threads are joined, then I don't think this is an issue. But I don't personally know a reliable way other than threading._register_atexit.
atexit.register only runs after threads are joined, not non-daemon threads (hence #86128), meaning that switching the thread to non-daemon will hang process exit until select returns, because no wake is called. A public hook that does exactly what threading._register_atexit does would be great. But since this is now in the stdlib, I could use that here, I suppose (#86128 remains a problem for non-stdlib code, of course).
matches tornado behavior tornado handles this in IOLoop, so we need to include it
- remove some older Python compatibility, since this is in the stdlib now - resolve race between selector thread call soon and EventLoop.close
rely on private threading._register_atexit to run cleanup prior to thread join
based on confusion in feedback
according to devguide
| When called, :func:`select.select` is run in an additional thread. | ||
|
|
||
| The resolution of the monotonic clock on Windows is usually around 15.6 | ||
| milliseconds. The best resolution is 0.5 milliseconds. The resolution depends on the |
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.
"The resolution of the monotonic clock on Windows is usually around 15.6 milliseconds."
Note unrelated to this PR: that's no longer true in Python 3.13:
On Windows, monotonic() now uses the QueryPerformanceCounter() clock for a resolution of 1 microsecond, instead of the GetTickCount64() clock which has a resolution of 15.6 milliseconds.
Co-authored-by: Victor Stinner <[email protected]>
Co-authored-by: Victor Stinner <[email protected]>
vstinner
left a comment
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.
Overall, the change looks good to me. I didn't test it. I just trust tests and the code that I read :-) I would prefer to have a second review from another core dev.
@kumaraditya303: Would you be interested to review this change?
| with mock.patch("select.select", wraps=select.select) as mock_select: | ||
| self.selector_thread.add_reader(b, mock_recv) | ||
| # ready event, but main event loop is blocked for some time | ||
| time.sleep(0.1) |
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.
Is this sleep really needed? If yes, can it be replaced with a synchronization primitive instead of a sleep?
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 would love help writing tests to verify that things don't happen in multithreaded code.
This was added to demonstrate an answer to the question in review that the select loop doesn't keep firing away if the main loop is slow to process one event, and was the best I could come up with to demonstrate that it really is waiting and only calls select once.
The sequence of events that we want to wait for is that the select thread is waiting in select_cond.wait() after select is called the first time. Maybe there's a way to instrument that.
Runs
select.selectin a background thread whenadd_reader/writer are called. Thread is terminated when the event loop is destroyed.Solves a major problem with the default event loop lacking these methods (see #81554 for use cases).
Imports
SelectorThreadimplementation toasyncio._selector_threadfrom Tornado 6.5.2 and uses it in ProactorEventLoop, fixing a longstanding compatibility problem with the default Windows event loop. Feel free to review_selector_thread, but note that it currently has almost no modifications to the original file from tornado (only removing some type hints not supported by the standard library).Since this is only run on demand, no thread will be spawned for existing code that works on ProactorEventLoop, only new code that previously only worked on SelectorEventLoop or tornado's AddThreadEventLoop will now work with the default event loop.
It is possible to do this without a thread (
triodoes it), but it seems this is the simplest, most maintainable solution, as indicated in the discussion in #81554.📚 Documentation preview 📚: https://cpython-previews--141834.org.readthedocs.build/