-
-
Notifications
You must be signed in to change notification settings - Fork 30.5k
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
deprecate the asyncio child watchers system #94597
Comments
Some background to this proposed deprecation: This is a more comprehensive version of #82772 @serhiy-storchaka proposed deprecating
@asvetlov noted that
|
We are in dire need of more asyncio experts. @1st1 this isn't urgent but would be nice to have your perspective in time to do this in 3.12. |
For 3.12 IMO we should deprecate |
We discussed this at the sprint and we agree that there are many things wrong with the child watchers and the policy system. Deprecating child watchers: @1st1 thinks these should be done per loop (like uvloop does), not globally by the policy. Much more discussion on this topic is already in #82772. Bottom line, we agree to deprecate it, details remain to be seen. Deprecating policies: Yes please. The policies no longer serve a real purpose. Loops are always per thread, there is no need to have a "current loop" when no loop is currently running. The only thing we still need is a loop factory, so perhaps instead of an API for getting/setting a global "policy", we could have an API for getting/setting a global "loop factory". I'm fine with the We should totally deprecate |
I disagree, that's the most painful part of the policy system that I'm looking to deprecate here in favor of passing an explicit |
What exactly is most painful? That there's a global default for something? To me the painful thing is that the policy system is over-engineered, you have to create a class that overrides I totally agree that most people should use |
I discussed this with Yury and he convinced me that we don't need a global loop factory. Instead we should just have a We still have to come up with a way to transition to a world where child watching is per-loop instead of global though. |
Good news. @1st1 has a simple refactoring of PidfdChildWatcher that makes it independent from the main loop -- just like ThreadedChildWatcher. Once that is merged (PR is forthcoming) we can also merge @kumaraditya303's PR GH-98024, and then we can start deprecating all other child watcher implementations. We can then also deprecate set_child_watcher() (both the asyncio function and the policy method) and eventually we can move the child watcher out of the policy. There's some hand-waving here because in theory people could subclass the default policy class and override get_child_watcher() to construct their own child watcher -- we'll have to deprecate that too somehow. But all this builds a road to a world where policies are no longer needed and eventually no longer exist. |
@gvanrossum I have a PR for Pidfdchildwatcher already #94184 |
It would also be good for the child watchers to be responsible for calling the callback on the event loop thread. Currently the callback needs to defensively call Eg cpython/Lib/asyncio/unix_events.py Line 227 in 282edd7
|
I just read the comments in GH-93453: "Make get_event_loop() an alias of get_running_loop()". This makes me want to go slow with the whole "deprecate policies" part. (I am still fine with deprecating watchers ASAP.) Maybe we could start by deprecating just |
Let's deprecate child watchers first and then we can think about policy since it will require more discussion. |
FWIW it seems that Jupyter has a legitimate reason to override the default policy (with another one of the predefined ones), see #93453 (comment). |
The WindowsSelectorEventLoopPolicy shouldn't be needed with tornado 6.2 where it runs a selector in a background thread so the main event loop can be the ProactorEventLoop |
It seems @1st1 is in favor of deprecating the rest of the policy system: https://twitter.com/1st1/status/1711007413275365590?t=PGAYW5447_2JZdgiIsu6eQ&s=19 can we do this for 3.13? |
I am fine with that. But I am not someone with a lot of time for it. In 3.13 all we need is some deprecations. Can you help with that? |
@gvanrossum I can definitely help adding deprecations! |
This is needed to pave the way for deprecating and eventually killing the event loop policy system (which is over-engineered and rarely used).
I want to cleanup the calls to def teardownModule():
asyncio.set_event_loop_policy(None) this is now possible for tests that just use |
I attempted to bring Chaperone up to 3.11 last year, the reason it uses a custom watcher and the reason I was looking at them again is that anyone wanting to install a SIGCHLD handler under asyncio is going to stumble onto the conflict it produces with the child watchers. Something designed to run as pid 1 should really be doing zombie reaping for unknown processes and a custom watcher seems to be the only reliable way to install a default behaviour. Prior to 3.8 the default was SafeChildWatcher, which has a conflict, so a custom was required. ThreadedChildWatcher from 3.8 and PidfdChildWatcher from 3.9 seem like they won't conflict, however only the former offers a tracking mechanism via The proper method of dealing with this (admittedly niche) use case still seems to be a custom child watcher or not using asyncio at all. If I'm interpreting the comments correctly, this will now require an entire policy instead of being able to just override the watcher? |
This is complete, the deprecation of policy system can be done in separate issue. |
# Description Replace standard child watcher with FastChildWatcher This will cause it to also clean up zombie processes, but we will have to replace this in the near future, as this while subsystem is deprecated python/cpython#94597. # Self Check: Strike through any lines that are not applicable (`~~line~~`) then check the box - [ ] Attached issue to pull request - [x] Changelog entry - [x] Type annotations are present - [x] Code is clear and sufficiently documented - [x] No (preventable) type errors (check using make mypy or make mypy-diff) - [x] Sufficient test cases (reproduces the bug/tests the requested feature) - [x] Correct, in line with design - [ ] End user documentation is included or an issue is created for end-user documentation (add ref to issue here: ) - [ ] If this PR fixes a race condition in the test suite, also push the fix to the relevant stable branche(s) (see [test-fixes](https://internal.inmanta.com/development/core/tasks/build-master.html#test-fixes) for more info)
This is needed to pave the way for deprecating and eventually killing the event loop policy system (which is over-engineered and rarely used).
Deprecate the child watchers system and the policy system in favour of
asyncio.Runner(loop_factory=asyncio.ProactorEventLoop/asyncio.SelectorEventLoop/uvloop.new_event_loop)
that would be deprecating:
I'd also like to introduce a new API:
asyncio.EventLoop
implemented as:asyncio.new_event_loop()
will issue a DeprecationWarning if the current policy is not the default policy, and then in 3 releases become an alias ofasyncio.EventLoop
Originally posted by @graingert in #93896 (comment)
AbstractChildWatcher
#99386The text was updated successfully, but these errors were encountered: