Repository: aio-libs/aiohttp
Pull request: Add keepalive_timeout parameter to web.run_app
<!-- Thank you for your contribution! -->

## What do these changes do?

Add keepalive_timeout parameter to web.run_app. Closes #5094.

<!-- Please give a short brief about these changes. -->

## Are there changes in behavior for the user?

Unlikely.

<!-- Outline any notable behaviour for the end users. -->

## Related issue number

<!-- Are there any issues opened that will be resolved by merging this change? -->

## Checklist

- [x] I think the code is well written
- [x] Unit tests for the changes exist
- [x] Documentation reflects the changes
- [x] If you provide code modification, please add yourself to `CONTRIBUTORS.txt`
  * The format is &lt;Name&gt; &lt;Surname&gt;.
  * Please keep alphabetical order, the file is sorted by names. 
- [x] Add a new news fragment into the `CHANGES` folder
  * name it `<issue_id>.<type>` for example (588.bugfix)
  * if you don't have an `issue_id` change it to the pr id after creating the pr
  * ensure type is one of the following:
    * `.feature`: Signifying a new feature.
    * `.bugfix`: Signifying a bug fix.
    * `.doc`: Signifying a documentation improvement.
    * `.removal`: Signifying a deprecation or removal of public API.
    * `.misc`: A ticket has been closed, but it is not of interest to users.
  * Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."


Problem statement:
Allow configuring the keepalive timeout in web.run_app
🐣 **Is your feature request related to a problem? Please describe.**
<!-- A clear and concise description of what the problem is. Ex. I'm always frustrated when [...] -->
Adjusting the keepalive timeout is not possible without copy-pasting a lot of boilerplate code usually supplied by the `web.run_app` function.

💡 **Describe the solution you'd like**
<!-- A clear and concise description of what you want to happen. -->
Add a `keepalive_timeout` parameter to the `web.run_app` function.

Hints from issue discussion:

Commit message: Fix call_args usage for Python < 3.8

Patch to review:
diff --git a/CHANGES/5094.feature b/CHANGES/5094.feature
new file mode 100644
index 00000000000..2d140f175e9
--- /dev/null
+++ b/CHANGES/5094.feature
@@ -0,0 +1 @@
+Add keepalive_timeout parameter to web.run_app.
diff --git a/CONTRIBUTORS.txt b/CONTRIBUTORS.txt
index 93d4ab5b1ed..35e2ae2e074 100644
--- a/CONTRIBUTORS.txt
+++ b/CONTRIBUTORS.txt
@@ -141,6 +141,7 @@ Jaesung Lee
 Jake Davis
 Jakob Ackermann
 Jakub Wilk
+Jan Buchar
 Jashandeep Sohi
 Jens Steinhauser
 Jeonghun Lee
diff --git a/aiohttp/web.py b/aiohttp/web.py
index a5de6fcee54..e655b3f1085 100644
--- a/aiohttp/web.py
+++ b/aiohttp/web.py
@@ -292,6 +292,7 @@ async def _run_app(app: Union[Application, Awaitable[Application]], *,
                    path: Optional[str]=None,
                    sock: Optional[socket.socket]=None,
                    shutdown_timeout: float=60.0,
+                   keepalive_timeout: float=75.0,
                    ssl_context: Optional[SSLContext]=None,
                    print: Optional[Callable[..., None]]=print,
                    backlog: int=128,
@@ -310,7 +311,8 @@ async def _run_app(app: Union[Application, Awaitable[Application]], *,
     runner = AppRunner(app, handle_signals=handle_signals,
                        access_log_class=access_log_class,
                        access_log_format=access_log_format,
-                       access_log=access_log)
+                       access_log=access_log,
+                       keepalive_timeout=keepalive_timeout)
 
     await runner.setup()
 
@@ -407,6 +409,7 @@ def run_app(app: Union[Application, Awaitable[Application]], *,
             path: Optional[str]=None,
             sock: Optional[socket.socket]=None,
             shutdown_timeout: float=60.0,
+            keepalive_timeout: float=75.0,
             ssl_context: Optional[SSLContext]=None,
             print: Optional[Callable[..., None]]=print,
             backlog: int=128,
@@ -435,6 +438,7 @@ def run_app(app: Union[Application, Awaitable[Application]], *,
             path=path,
             sock=sock,
             shutdown_timeout=shutdown_timeout,
+            keepalive_timeout=keepalive_timeout,
             ssl_context=ssl_context,
             print=print,
             backlog=backlog,
diff --git a/docs/web_reference.rst b/docs/web_reference.rst
index 297a3a10d4e..2cf485c4123 100644
--- a/docs/web_reference.rst
+++ b/docs/web_reference.rst
@@ -2737,7 +2737,8 @@ Utilities
 
 .. function:: run_app(app, *, debug=False, host=None, port=None, \
                       path=None, sock=None, shutdown_timeout=60.0, \
-                      ssl_context=None, print=print, backlog=128, \
+                      keepalive_timeout=75.0, ssl_context=None, \
+                      print=print, backlog=128, \
                       access_log_class=aiohttp.helpers.AccessLogger, \
                       access_log_format=aiohttp.helpers.AccessLogger.LOG_FORMAT, \
                       access_log=aiohttp.log.access_logger, \
@@ -2793,6 +2794,10 @@ Utilities
                                 timeout but closes a server in a few
                                 milliseconds.
 
+   :param float keepalive_timeout: a delay before a TCP connection is
+                                   closed after a HTTP request. The delay
+                                   allows for reuse of a TCP connection.
+
    :param ssl_context: :class:`ssl.SSLContext` for HTTPS server,
                        ``None`` for HTTP connection.
 
diff --git a/tests/test_run_app.py b/tests/test_run_app.py
index efc79cfc2c5..92bba9a9f01 100644
--- a/tests/test_run_app.py
+++ b/tests/test_run_app.py
@@ -16,6 +16,7 @@
 from aiohttp import web
 from aiohttp.helpers import PY_37
 from aiohttp.test_utils import make_mocked_coro
+from aiohttp.web_runner import BaseRunner
 
 # Test for features of OS' socket support
 _has_unix_domain_socks = hasattr(socket, 'AF_UNIX')
@@ -685,6 +686,24 @@ async def on_startup(app):
     exc_handler.assert_called_with(patched_loop, msg)
 
 
+def test_run_app_keepalive_timeout(patched_loop):
+    new_timeout = 1234
+    base_runner_init_mock = mock.MagicMock()
+    base_runner_init_orig = BaseRunner.__init__
+
+    def base_runner_init_spy(self, *args, **kwargs):
+        base_runner_init_mock(*args, **kwargs)
+        base_runner_init_orig(self, *args, **kwargs)
+
+    with mock.patch.object(BaseRunner, "__init__", base_runner_init_spy):
+        app = web.Application()
+        web.run_app(app, keepalive_timeout=new_timeout,
+                    print=stopper(patched_loop))
+
+    base_runner_init_kwargs = base_runner_init_mock.call_args[1]
+    assert (base_runner_init_kwargs["keepalive_timeout"] == new_timeout)
+
+
 @pytest.mark.skipif(not PY_37,
                     reason="contextvars support is required")
 def test_run_app_context_vars(patched_loop):


Code review comments:
diff_hunk: @@ -685,6 +686,24 @@ async def on_startup(app):
     exc_handler.assert_called_with(patched_loop, msg)
 
 
+def test_run_app_keepalive_timeout(patched_loop):
+    new_timeout = 1234
+    base_runner_init_mock = mock.MagicMock()
+    base_runner_init_orig = BaseRunner.__init__
+
+    def base_runner_init_spy(self, *args, **kwargs):
+        base_runner_init_mock(*args, **kwargs)
+        base_runner_init_orig(self, *args, **kwargs)
+
+    with mock.patch.object(BaseRunner, "__init__", base_runner_init_spy):
path: tests/test_run_app.py
text: @user1:
It's better to use the built-in [`monkeypatch` fixture](https://docs.pytest.org/en/stable/monkeypatch.html) for patching things.
Also, it's best to only patch the line that relies on patching, not many lines because you may accidentally create unintended side-effects with this.

diff_hunk: @@ -685,6 +686,24 @@ async def on_startup(app):
     exc_handler.assert_called_with(patched_loop, msg)
 
 
+def test_run_app_keepalive_timeout(patched_loop):
path: tests/test_run_app.py
text: @user1:
It's usually better to use the [`mocker`](https://pypi.org/project/pytest-mock/#usage) fixture from `pytest-mock` (it's already integrated as a test dependency) for accessing the `MagicMock` class. 

@author:
Sure, I can update it. What exactly is the benefit of using the fixture?

@user1:
Just the consistency: if the lib is integrated, the expectation is that it would be used. Also, pytest design is more fixture-driven so external things seem foreign.

@author:
Got it. I can change this (and the rest of the comments) if @user2 agrees with the feature itself.

diff_hunk: @@ -685,6 +686,24 @@ async def on_startup(app):
     exc_handler.assert_called_with(patched_loop, msg)
 
 
+def test_run_app_keepalive_timeout(patched_loop):
+    new_timeout = 1234
+    base_runner_init_mock = mock.MagicMock()
+    base_runner_init_orig = BaseRunner.__init__
+
+    def base_runner_init_spy(self, *args, **kwargs):
+        base_runner_init_mock(*args, **kwargs)
+        base_runner_init_orig(self, *args, **kwargs)
+
+    with mock.patch.object(BaseRunner, "__init__", base_runner_init_spy):
+        app = web.Application()
+        web.run_app(app, keepalive_timeout=new_timeout,
+                    print=stopper(patched_loop))
+
+    base_runner_init_kwargs = base_runner_init_mock.call_args[1]
+    assert (base_runner_init_kwargs["keepalive_timeout"] == new_timeout)
path: tests/test_run_app.py
text: @user1:
assert is not a function and so it does not require parentheses: 
```suggestion
    assert base_runner_init_kwargs["keepalive_timeout"] == new_timeout
```

@author:
The assert used to be broken across two lines and the parentheses remained there.

diff_hunk: @@ -685,6 +686,24 @@ async def on_startup(app):
     exc_handler.assert_called_with(patched_loop, msg)
 
 
+def test_run_app_keepalive_timeout(patched_loop):
+    new_timeout = 1234
+    base_runner_init_mock = mock.MagicMock()
+    base_runner_init_orig = BaseRunner.__init__
+
+    def base_runner_init_spy(self, *args, **kwargs):
+        base_runner_init_mock(*args, **kwargs)
path: tests/test_run_app.py
text: @user1:
It looks like it'd be much cleaner to just do
```suggestion
        assert kwargs["keepalive_timeout"] == new_timeout
```