-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Implement web.Runner
context manager
#8723
base: master
Are you sure you want to change the base?
Conversation
import asyncio | ||
import contextvars | ||
import re | ||
import unittest |
Check notice
Code scanning / CodeQL
Module is imported with 'import' and 'import from' Note test
try: | ||
await asyncio.sleep(0.1) | ||
except asyncio.CancelledError: | ||
1 / 0 |
Check notice
Code scanning / CodeQL
Statement has no effect Note test
async for the_meaning_of_life in spinner: | ||
pass | ||
except asyncio.CancelledError: | ||
1 / 0 |
Check notice
Code scanning / CodeQL
Statement has no effect Note test
cvar.set(val) | ||
return old | ||
|
||
async def get_context(): |
Check notice
Code scanning / CodeQL
Unused local variable Note test
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 like a good start. Still needs some fixes and documentation.
self._loop.set_debug(self._debug) | ||
|
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 already done in _laze_init().
self._loop.set_debug(self._debug) |
if self._set_event_loop: | ||
asyncio.set_event_loop(self._loop) |
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.
Also in _lazy_init()
if self._set_event_loop: | |
asyncio.set_event_loop(self._loop) |
_cancel_tasks({main_task}, self._loop) | ||
_cancel_tasks(asyncio.all_tasks(self._loop), self._loop) | ||
self._loop.run_until_complete(self._loop.shutdown_asyncgens()) | ||
self.close() | ||
asyncio.set_event_loop(None) |
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.
Already done in .close():
_cancel_tasks({main_task}, self._loop) | |
_cancel_tasks(asyncio.all_tasks(self._loop), self._loop) | |
self._loop.run_until_complete(self._loop.shutdown_asyncgens()) | |
self.close() | |
asyncio.set_event_loop(None) | |
_cancel_tasks({main_task}, self._loop) | |
self.close() |
if ( | ||
threading.current_thread() is threading.main_thread() | ||
and signal.getsignal(signal.SIGINT) is signal.default_int_handler | ||
): | ||
sigint_handler = functools.partial(self._on_sigint, main_task=task) | ||
try: | ||
signal.signal(signal.SIGINT, sigint_handler) | ||
except ValueError: | ||
# `signal.signal` may throw if `threading.main_thread` does | ||
# not support signals (e.g. embedded interpreter with signals | ||
# not registered - see gh-91880) | ||
sigint_handler = None | ||
else: | ||
sigint_handler = None |
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 wonder if this SIGINT code needs to be copied to run_app()...
Will have to do some more testing.
if loop is not None: | ||
|
||
def loop_factory(): | ||
return loop | ||
|
||
else: | ||
loop_factory = events.get_running_loop |
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 loop_factory should default to None (maybe this is the cause of the RuntimeError above?).
if loop is not None: | |
def loop_factory(): | |
return loop | |
else: | |
loop_factory = events.get_running_loop | |
loop_factory = None if loop is None else lambda: loop |
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.
We'll also want some tests in test_run_app.py to verify the .run_app() method separately.
e.g. Maybe test that running a simple task followed by .run_app() works (and happens in the same loop).
@bdraco Does this look reasonable overall? |
Also, looking at the implementation, the .run_app() method only uses ._lazy_init() and ._loop from the class. So, I'm wondering if we should just ignore the comment about the class being final and subclass it anyway. Then just rely on decent tests to ensure we maintain compatibility. |
Traveling today for the next 23 hours. Will take a look when I get to my destination |
Looks like I'll be able to look today after all. Flight isn't happening due to engine issues so I'll be able to look once I get rebooked and back home. |
It looks reasonable to me. I want to subclass it instead of copying all the code, as keeping it in sync imposes some maintenance burden. Unfortunately, upstream explicitly says not to do that. We should add some comments about periodically checking upstream for bug fixes if we don't want to ignore that and do it anyways since we will have an out-of-sync copy as soon as anything changes. |
Yeah, I think we should just subclass it, there's just too much code otherwise with tests duplicated etc. We only depend on lazy_init and the loop attribute from the class, so I think risk of breakage is low. @DavidRomanovizc Would you be alright changing it to a subclass and removing the copied tests?
Then in web.run_app() also do a version check and use the new Runner code in 3.11+ or the old implementation otherwise (we'll then remove the old version when we drop support for 3.10). |
What do these changes do?
These changes introduce the
web.Runner
context manager, based onasyncio.Runner
. Addedweb.Runner.run_app
method and refactored existingweb.run_app
to useweb.Runner.run_app
for running server. This feature preserves the backward compatibilityAre there changes in behavior for the user?
I think there aren't changes in behavior for users who will continue to use
web.run_app
as before; it remains fully compatible with existing code but users now have the option to useweb.Runner.run_app
Is it a substantial burden for the maintainers to support this?
Supporting this change shouldn't be a substantial burden for maintainers because
web.Runner
builds on theasyncio.Runner
with minor additionsRelated to Issue: #8028
Checklist
CONTRIBUTORS.txt
CHANGES/
folder