Implement `web.Runner` context manager #8723

DavidRomanovizc · 2024-08-16T18:42:14Z

What do these changes do?

These changes introduce the web.Runner context manager, based on asyncio.Runner. Added web.Runner.run_app method and refactored existing web.run_app to use web.Runner.run_app for running server. This feature preserves the backward compatibility

Are 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 use web.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 the asyncio.Runner with minor additions

Related to Issue: #8028

Checklist

I think the code is well written
Unit tests for the changes exist
Documentation reflects the changes (Before updating docs, I want to make sure that I'm on the right track)
If you provide code modification, please add yourself to CONTRIBUTORS.txt
- The format is <Name> <Surname>.
- Please keep alphabetical order, the file is sorted by names.
Add a new news fragment into the CHANGES/ folder

tests/test_runner.py

Dreamsorcerer

Looks like a good start. Still needs some fixes and documentation.

aiohttp/web.py

Dreamsorcerer · 2024-08-21T12:57:25Z

aiohttp/web.py

+        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


I wonder if this SIGINT code needs to be copied to run_app()...
Will have to do some more testing.

aiohttp/web.py

tests/test_runner.py

Dreamsorcerer · 2024-08-21T13:15:32Z

tests/test_runner.py

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).

Dreamsorcerer · 2024-08-21T13:22:35Z

@bdraco Does this look reasonable overall?
I think after merging this, I'll make a followup PR to move all of run_app/Runner code to a separate file, as this one is getting a bit too big.

Dreamsorcerer · 2024-08-21T13:27:39Z

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.

bdraco · 2024-08-21T14:18:45Z

Traveling today for the next 23 hours. Will take a look when I get to my destination

bdraco · 2024-08-21T15:46:06Z

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.

bdraco · 2024-08-21T22:52:44Z

@bdraco Does this look reasonable overall? I think after merging this, I'll make a followup PR to move all of run_app/Runner code to a separate file, as this one is getting a bit too big.

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.

Dreamsorcerer · 2024-08-22T13:40:06Z

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?
You'll need to define it with:

if sys.version_info >= (3, 11):
    class Runner(...

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).

Co-authored-by: Sam Bull <[email protected]>

DavidRomanovizc · 2024-10-27T06:21:12Z

Sorry for the long inactivity. Decided to rename Runner to WebRunner, inherited asyncio.Runner and added check version. In web.run_app returned old implementation and use the new Runner code if py version 3.11+ or the old impl otherwise

codspeed-hq · 2024-10-27T06:27:11Z

CodSpeed Performance Report

Merging #8723 will not alter performance

_{Comparing DavidRomanovizc:feature-runner (1f733e6) with master (dd9a1fd)}

Summary

✅ 4 untouched benchmarks

Dreamsorcerer · 2024-10-27T12:57:57Z

aiohttp/web.py

+    class WebRunner(asyncio.Runner):  # type: ignore
+        """A context manager that controls event loop life cycle"""
+
+        def __init__(


Still a lot of copied code here. As far as I can tell, only the run_app() method needs to be defined here. init/close/_lazy_init can all be removed and just use the parent methods.

Dreamsorcerer · 2024-10-27T13:02:28Z

aiohttp/web.py

+                if not access_log.hasHandlers():
+                    access_log.addHandler(logging.StreamHandler())
+
+            main_task = self._loop.create_task(


Should probably add the context parameter for compatibility:
https://github.com/python/cpython/blob/dc76a4ad3c46af3fb70361be10b8a791cd126931/Lib/asyncio/runners.py#L106-L109

aiohttp/web.py

Dreamsorcerer · 2024-10-27T13:12:22Z

aiohttp/web.py

+        INITIALIZED = "initialized"
+        CLOSED = "closed"
+
+    class WebRunner(asyncio.Runner):  # type: ignore


We should probably get a new test that uses WebRunner directly too. Can probably just copy and modify one of the run_app() tests. The new test could also use WebRunner.run() to verify that the task and the app are run in the same loop.

DavidRomanovizc · 2024-10-27T17:39:05Z

@Dreamsorcerer, I thought, why don't we replace inheritance with composition? We will still be able to use the functionality of asyncio.Runner. Also, we won't have to spend effort on maintenance because we ensure synchronization with the upstream

like that:

class WebRunner:
    def __init__(
        self,
        *,
        debug: Optional[bool] = None,
        loop_factory: Optional[Callable[[], asyncio.AbstractEventLoop]] = None,
    ):
        self.debug = debug
        self.loop_factory = loop_factory
        self._runner = asyncio.Runner(debug=debug, loop_factory=loop_factory)
   ...

Co-authored-by: Sam Bull <[email protected]>

Dreamsorcerer · 2024-10-28T12:42:31Z

I thought, why don't we replace inheritance with composition?

We need to call _lazy_init(), right? So, we can't just rely on the public interface..

DavidRomanovizc added 2 commits August 15, 2024 00:40

add web.Runner context manager

57afbfc

✅ add tests for web.Runner

283570f

DavidRomanovizc requested a review from asvetlov as a code owner August 16, 2024 18:42

📝 add newsfragment

77ec5eb

DavidRomanovizc requested a review from webknjaz as a code owner August 16, 2024 18:46

psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Aug 16, 2024

github-advanced-security bot found potential problems Aug 16, 2024

View reviewed changes

tests/test_runner.py Fixed Show fixed Hide fixed

tests/test_runner.py Fixed Show fixed Hide fixed

tests/test_runner.py Fixed Show fixed Hide fixed

tests/test_runner.py Fixed Show fixed Hide fixed

Dreamsorcerer requested changes Aug 21, 2024

View reviewed changes

Dreamsorcerer added the pr-unfinished The PR is unfinished and may need a volunteer to complete it label Sep 12, 2024

DavidRomanovizc and others added 5 commits October 27, 2024 07:30

remove redundant call self._loop.set_debug(self._debug)

50c1a6b

Co-authored-by: Sam Bull <[email protected]>

remove duplicate code

3df86f3

Co-authored-by: Sam Bull <[email protected]>

remove duplicate code

a917375

Co-authored-by: Sam Bull <[email protected]>

remove the copied tests

7db4f09

rename Runner to WebRunner and make it subclass of asyncio.Runner

1684a34

Merge branch 'master' into feature-runner

722631c

Dreamsorcerer removed the pr-unfinished The PR is unfinished and may need a volunteer to complete it label Oct 27, 2024

Dreamsorcerer reviewed Oct 27, 2024

View reviewed changes

aiohttp/web.py Outdated Show resolved Hide resolved

Dreamsorcerer reviewed Oct 27, 2024

View reviewed changes

fix version check

1f733e6

Co-authored-by: Sam Bull <[email protected]>

Dreamsorcerer added the pr-unfinished The PR is unfinished and may need a volunteer to complete it label Nov 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `web.Runner` context manager #8723

Implement `web.Runner` context manager #8723

DavidRomanovizc commented Aug 16, 2024 •

edited by Dreamsorcerer

Loading

Dreamsorcerer left a comment

Dreamsorcerer Aug 21, 2024

Dreamsorcerer Aug 21, 2024

Dreamsorcerer commented Aug 21, 2024

Dreamsorcerer commented Aug 21, 2024

bdraco commented Aug 21, 2024

bdraco commented Aug 21, 2024

bdraco commented Aug 21, 2024

Dreamsorcerer commented Aug 22, 2024 •

edited

Loading

DavidRomanovizc commented Oct 27, 2024

codspeed-hq bot commented Oct 27, 2024 •

edited

Loading

Dreamsorcerer Oct 27, 2024

Dreamsorcerer Oct 27, 2024

Dreamsorcerer Oct 27, 2024

DavidRomanovizc commented Oct 27, 2024

Dreamsorcerer commented Oct 28, 2024

Implement web.Runner context manager #8723

Are you sure you want to change the base?

Implement web.Runner context manager #8723

Conversation

DavidRomanovizc commented Aug 16, 2024 • edited by Dreamsorcerer Loading

What do these changes do?

Are there changes in behavior for the user?

Is it a substantial burden for the maintainers to support this?

Checklist

Dreamsorcerer left a comment

Choose a reason for hiding this comment

Dreamsorcerer Aug 21, 2024

Choose a reason for hiding this comment

Dreamsorcerer Aug 21, 2024

Choose a reason for hiding this comment

Dreamsorcerer commented Aug 21, 2024

Dreamsorcerer commented Aug 21, 2024

bdraco commented Aug 21, 2024

bdraco commented Aug 21, 2024

bdraco commented Aug 21, 2024

Dreamsorcerer commented Aug 22, 2024 • edited Loading

DavidRomanovizc commented Oct 27, 2024

codspeed-hq bot commented Oct 27, 2024 • edited Loading

CodSpeed Performance Report

Merging #8723 will not alter performance

Summary

Dreamsorcerer Oct 27, 2024

Choose a reason for hiding this comment

Dreamsorcerer Oct 27, 2024

Choose a reason for hiding this comment

Dreamsorcerer Oct 27, 2024

Choose a reason for hiding this comment

DavidRomanovizc commented Oct 27, 2024

Dreamsorcerer commented Oct 28, 2024

Implement `web.Runner` context manager #8723

Implement `web.Runner` context manager #8723

DavidRomanovizc commented Aug 16, 2024 •

edited by Dreamsorcerer

Loading

Dreamsorcerer commented Aug 22, 2024 •

edited

Loading

codspeed-hq bot commented Oct 27, 2024 •

edited

Loading