Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add keepalive_timeout parameter to web.run_app #5095

Merged
merged 16 commits into from
Oct 27, 2020
Merged

Add keepalive_timeout parameter to web.run_app #5095

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
c8ccce0
Add keepalive_timeout parameter to web.run_app
janbuchar Oct 20, 2020
fcc2253
Add test for setting keepalive_timeout
janbuchar Oct 20, 2020
827375f
Code format
janbuchar Oct 20, 2020
2a99b5c
A more complex way to spy on BaseRunner.__init__
janbuchar Oct 20, 2020
df61102
Update doc
janbuchar Oct 20, 2020
dfa3639
Update CONTRIBUTORS.txt
janbuchar Oct 20, 2020
427489c
Add missing full stop
janbuchar Oct 20, 2020
7a84d8d
Merge branch 'master' into master
janbuchar Oct 20, 2020
fcc6de6
Fix call_args usage for Python < 3.8
janbuchar Oct 20, 2020
998fb71
Update tests/test_run_app.py
janbuchar Oct 20, 2020
f136cd9
Simplify test
janbuchar Oct 26, 2020
2a33550
Update docs/web_reference.rst
janbuchar Oct 26, 2020
4ecfaf8
Change versionadded for the keepalive_timeout parameter
janbuchar Oct 27, 2020
ccefa65
Fix code style
janbuchar Oct 27, 2020
beee1d5
Merge branch 'master' into master
janbuchar Oct 27, 2020
4dda278
Apply suggestions from code review
janbuchar Oct 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGES/5094.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add keepalive_timeout parameter to web.run_app.
1 change: 1 addition & 0 deletions CONTRIBUTORS.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ Jaesung Lee
Jake Davis
Jakob Ackermann
Jakub Wilk
Jan Buchar
Jashandeep Sohi
Jens Steinhauser
Jeonghun Lee
Expand Down
6 changes: 5 additions & 1 deletion aiohttp/web.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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()

Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
7 changes: 6 additions & 1 deletion docs/web_reference.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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, \
Expand Down Expand Up @@ -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
asvetlov marked this conversation as resolved.
Show resolved Hide resolved
closed after a HTTP request. The delay
allows for reuse of a TCP connection.

janbuchar marked this conversation as resolved.
Show resolved Hide resolved
:param ssl_context: :class:`ssl.SSLContext` for HTTPS server,
``None`` for HTTP connection.

Expand Down
19 changes: 19 additions & 0 deletions tests/test_run_app.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down Expand Up @@ -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):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's usually better to use the mocker fixture from pytest-mock (it's already integrated as a test dependency) for accessing the MagicMock class.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like it'd be much cleaner to just do

Suggested change
base_runner_init_mock(*args, **kwargs)
assert kwargs["keepalive_timeout"] == new_timeout

base_runner_init_orig(self, *args, **kwargs)
webknjaz marked this conversation as resolved.
Show resolved Hide resolved

with mock.patch.object(BaseRunner, "__init__", base_runner_init_spy):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to use the built-in monkeypatch fixture 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.

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):
Expand Down