Implement web.Runner context manager
#8723
Conversation
psf-chronographer
bot
added
the
bot:chronographer:provided
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| 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
| self._loop.set_debug(self._debug) | ||
|
|
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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). |
Dreamsorcerer
added
the
pr-unfinished
What do these changes do?
These changes introduce the
web.Runnercontext manager, based onasyncio.Runner. Addedweb.Runner.run_appmethod and refactored existingweb.run_appto useweb.Runner.run_appfor 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_appas before; it remains fully compatible with existing code but users now have the option to useweb.Runner.run_appIs it a substantial burden for the maintainers to support this?
Supporting this change shouldn't be a substantial burden for maintainers because
web.Runnerbuilds on theasyncio.Runnerwith minor additionsRelated to Issue: #8028
Checklist
CONTRIBUTORS.txtCHANGES/folder