Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork34k
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 ourterms of service andprivacy 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?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
v6.5.1License: Apache-2.0Copyright (c) 2025 The Tornado Authorsunmodified from original, so we can track changes
via background SelectorThread, imported from tornado
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| # 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 inhttps://github.com/python/cpython/blob/main/Misc/sbom.spdx.json?
See thedevguide.
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, likeLib/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 <89152624+StanFromIreland@users.noreply.github.com>
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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
withloop._select_cond:loop._closing_selector=Trueloop._select_cond.notify()try:loop._waker_w.send(b"a")exceptBlockingIOError:pass
prior tothread.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 privatethreading._register_atexit.
I believe this is only needed to handle process teardown for unclosed event loops. If we can strictly guarantee thatEventLoop.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 thanthreading._register_atexit.
atexit.register only runsafter threads are joined, not non-daemon threads (hence#86128), meaning that switching the thread to non-daemon will hang process exit untilselect returns, because no wake is called. A public hook that does exactly whatthreading._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).
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
matches tornado behaviortornado 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'sno 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.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Victor Stinner <vstinner@python.org>
Co-authored-by: Victor Stinner <vstinner@python.org>
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 changelooks 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 thingsdon'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.
Uh oh!
There was an error while loading.Please reload this page.
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 hasalmost 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.
Itis 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/