Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.7k
gh-110771: Decompose run_forever() into parts#110773
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
gvanrossum commentedOct 12, 2023
Since you’re planning to use this, you do need tests and docs. |
gvanrossum 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.
Next, tests, I presume?
Lib/asyncio/base_events.py Outdated
| """Set up an event loop so that it is ready to start actively looping | ||
| to process events. |
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.
By convention the first paragraph of a docstring must be a one-line summary.
| """Setupaneventloopsothatitisreadytostartactivelylooping | |
| toprocessevents. | |
| """Prepareforprocessingevents. |
Lib/asyncio/base_events.py Outdated
| Returns the state that must be restored when the loop concludes. This state | ||
| should be passed in as arguments to ``run_forever_cleanup()``. |
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.
Document the type of the state, please. (If it's meant to be opaque, make it more opaque than a 1-tuple please. :-)
I find it a bit odd that the method name is "run_forever_setup()", which doesn't hint at the existence of a return value. (Could you store the state to be restored in a private/protected instance variable instead of returning it?)
Also, "arguments"? Or "argument"?
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've modified the implementation to store the state as a protected variable, which simplifies the interface.
Lib/asyncio/base_events.py Outdated
| Returns the state that must be restored when the loop concludes. This state | ||
| should be passed in as arguments to ``run_forever_cleanup()``. | ||
| This method is only needed if you are writing your own event loop, 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.
"is only needed if" -- odd phrasing. The method exists. Do you mean it only needs to be overridden? Maybe show a brief example of how it's supposed to be used in that case? (I intentionally didn't the docs before reading the code, and from only reading the docstring I'm a bit confused about what's hooking what. From reading more code I realize that setup/cleanup are what you intend to override.)
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.
No - it would be run_forever() that is being replaced. I've provided a sample implementation in the docs; I'll clarify the language here to (hopefully) make the intended usage more clear.
Lib/asyncio/base_events.py Outdated
| """Clean up an event loop after the event loop finishes the looping over | ||
| events. |
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.
| """Cleanupaneventloopaftertheeventloopfinishestheloopingover | |
| events. | |
| """Cleanupaftertheeventloopfinishestheloopingoverevents. |
| sys.set_asyncgen_hooks(*old_agen_hooks) | ||
| defrun_forever(self): | ||
| """Run until stop() is called.""" |
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.
Maybe clarify in this docstring that this is what calls setup/cleanup?
Doc/library/asyncio-eventloop.rst Outdated
| processing events. | ||
| Returns the state that must be restored when the loop concludes. This state | ||
| should be passed in as arguments to:meth:`loop.run_forever_cleanup()`. |
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.
"argument" I presume?
| should be passed in asarguments to:meth:`loop.run_forever_cleanup()`. | |
| should be passed in asargument to:meth:`loop.run_forever_cleanup()`. |
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.
No longer relevant as I've moved this to be an internal variable.
Doc/library/asyncio-eventloop.rst Outdated
| ..note:: | ||
| This method is only needed if you are writing your own ``EventLoop`` |
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.
Maybe this? Or maybe I misunderstand and the subclass wouldcall this and the cleanup method?
| This method is only needed if you are writing your own ``EventLoop`` | |
| Overriding this method is only needed if you are writing your own ``EventLoop`` |
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 usage I'm envisaging (and the usage that would be needed by Toga would be tocall the startup/cleanup methods, but provide a custom "while True: process event" loop that does whatever is needed for integration.
Overriding may also be required - the Windows event loop does this. However, I suspect that's more a case of Python's implementation using subclassing to satisfy the needs of all platforms, rather than something you'll see with GUI integration.
Doc/library/asyncio-eventloop.rst Outdated
| you are integrating Python's asyncio event loop with a GUI library's event | ||
| loop, you can use this method to ensure that Python's event loop is | ||
| correctly configured and ready to start processing individual events. Most | ||
| end users will not need to use this method directly. |
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.
Maybe stronger?
| end userswill notneed to use this method directly. | |
| end usersshould notcall this method directly. |
Also below.
Also, I'd like to see a tiny example of how to write such a subclass.
Doc/library/asyncio-eventloop.rst Outdated
| Perform any cleanup necessary at the conclusion of event processing to ensure | ||
| that the event loop has been fully shut down. | ||
| The *original_state* argument is the return value provided by the call to |
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 *original_state* argument is the return valueprovided by the call to | |
| The *original_state* argument is the return valuefrom the call to |
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.
No longer relevant as I've moved this to be an internal variable.
Doc/library/asyncio-eventloop.rst Outdated
| you are integrating Python's asyncio event loop with a GUI library's event | ||
| loop, you can use this method to ensure that Python's event loop has been | ||
| fully shut down at the conclusion of processing events. Most end users | ||
| will not need to use this method directly. |
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.
Same as above.
gvanrossum 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.
Getting very close...
Doc/library/asyncio-eventloop.rst Outdated
| ..method::loop.run_forever_cleanup() | ||
| Perform any cleanup necessary at the conclusion of event processing to ensure | ||
| that the event loop has been fully shut down. |
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.
Maybe say something about whether this is re-entrant? Should I worry about not calling it twice?
Doc/library/asyncio-eventloop.rst Outdated
| self.run_forever_cleanup() | ||
| gui_library.cleanup() |
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 imagine these two should happen in the reverse order? Cleanup in reverse order of setup. (Also, to be more robust, I'd use multiple nestedtry/finallys, but the example doesn't need that complexity.
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'll reverse the order; agreed that another layer of try/finally is probably overkill for documenation purposes.
Doc/library/asyncio-eventloop.rst Outdated
| self._run_once() | ||
| gui_library.process_events() | ||
| if self._stopping: |
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.
It's unfortunate you need to use private (technically "protected") API and variable here.
Then again, maybe "private" is the right term and we're okay with subclasses using those.
It's also unfortunate that this documentation now constrains what we can do inrun_forever().
It almost seems that we might as well document whatrun_forever() does and specify it as never doing anything else.
freakboy3742 commentedOct 13, 2023
Following conversation with@1st1 - making the setup/cleanup methods public API is a bit risky, as it locks us in to specific API designs around run loop operation. He suggested that doing the refactoring as a protected APIs was fine is a useful convenience for people who are subclassing in full knowledge that they're messing with internals. |
gvanrossum 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.
LGTM! Thanks for indulging me. Sorry to send you on a detour. Glad@1st1 knew what to do.
Effectively introduce an unstable, private (really: protected) API for subclasses that want to override `run_forever()`.
Incorporate the new run_forever integrations frompython/cpython#110773
Effectively introduce an unstable, private (really: protected) API for subclasses that want to override `run_forever()`.
Uh oh!
There was an error while loading.Please reload this page.
Decomposes
run_forever()into arun_forever_setup(), the actual loop, andrun_forever_cleanup(), so that the CPython asyncio event loop can be easily integrated with other event loops.Refactors the Winforms ProactorEventLoop's
run_forever()to make use of this new API entry point.I don't believe any additional testing is required, as coverage inside Python won't change.
Fixes#110771.