Add keepalive_timeout parameter to web.run_app by janbuchar · Pull Request #5095 · aio-libs/aiohttp

janbuchar · 2020-10-20T08:24:44Z

What do these changes do?

Add keepalive_timeout parameter to web.run_app. Closes #5094.

Are there changes in behavior for the user?

Unlikely.

Related issue number

Checklist

I think the code is well written
Unit tests for the changes exist
Documentation reflects the changes
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
- 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."

codecov-io · 2020-10-20T11:29:07Z

Codecov Report

Merging #5095 into master will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #5095      +/-   ##
==========================================
+ Coverage   97.63%   97.64%   +0.01%     
==========================================
  Files          43       43              
  Lines        8999     8999              
  Branches     1413     1413              
==========================================
+ Hits         8786     8787       +1     
+ Misses         99       98       -1     
  Partials      114      114

Flag	Coverage Δ
#unit	`97.64% <ø> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
aiohttp/web.py	`99.52% <ø> (ø)`
aiohttp/web_fileresponse.py	`98.38% <0.00%> (+0.53%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a8bf3a...fcc6de6. Read the comment docs.

webknjaz

See the initial review from me below. It's non-blocking but I don't have the confidence to accept this change on my own either and thus I defer the decision on whether it's something that we want to @asvetlov.

webknjaz · 2020-10-20T14:22:23Z

tests/test_run_app.py

    exc_handler.assert_called_with(patched_loop, msg)


+def test_run_app_keepalive_timeout(patched_loop):


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.

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

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.

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

webknjaz · 2020-10-20T14:24:51Z

tests/test_run_app.py

+        base_runner_init_mock(*args, **kwargs)
+        base_runner_init_orig(self, *args, **kwargs)
+
+    with mock.patch.object(BaseRunner, "__init__", base_runner_init_spy):


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.

tests/test_run_app.py

webknjaz · 2020-10-20T14:29:18Z

tests/test_run_app.py

+    base_runner_init_orig = BaseRunner.__init__
+
+    def base_runner_init_spy(self, *args, **kwargs):
+        base_runner_init_mock(*args, **kwargs)


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

docs/web_reference.rst

Co-authored-by: Sviatoslav Sydorenko <[email protected]>

docs/web_reference.rst

asvetlov · 2020-10-21T08:17:00Z

@webknjaz please feel free to merge the PR when you are okay with it.

Co-authored-by: Andrew Svetlov <[email protected]>

asvetlov · 2020-10-26T14:06:23Z

docs/web_reference.rst

                                   closed after a HTTP request. The delay
                                   allows for reuse of a TCP connection.

+      .. versionadded:: 3.7


Suggested change

.. versionadded:: 3.7

.. versionadded:: 3.8

Sorry, the time window for 3.1 is closed.

Understood.

janbuchar · 2020-10-27T07:48:19Z

@asvetlov is there any agreed-upon autoformatting tool that could help me resolve the conflicts?

asvetlov · 2020-10-27T10:59:02Z

You can try the following:

Copy https://github.com/aio-libs/aiohttp/blob/master/.pre-commit-config.yaml to your local repository.
Install pre-commit: pip install -U pre-commit
Apply all hooks: pre-commit run --all-files Repeat the step several times until all checks are green.
I hope your local copy will be in sync after reformatting; git merge upstream/master should be succeeded. If not -- the only manual conflict resolving can help

tests/test_run_app.py

Co-authored-by: Sviatoslav Sydorenko <[email protected]>

github-actions · 2020-10-27T20:12:01Z

💔 Backport was not successful

The PR was attempted backported to the following branches:

❌ 3.8: Commit could not be cherrypicked due to conflicts

PR #5095 by @Teyras Co-authored-by: Sviatoslav Sydorenko <[email protected]> Co-authored-by: Andrew Svetlov <[email protected]>

asvetlov · 2020-10-29T08:21:00Z

@webknjaz please backport manually next time if the bot fails.
Or, at least, let me know that I should do it myself

webknjaz · 2020-10-29T09:29:41Z

Oh, I probably missedmisse that among my notifications. I think it's also a good idea to encourage the submitters to do their backports when they are as interested in the change.

asvetlov · 2020-10-29T09:39:26Z

I think it's also a good idea to encourage the submitters to do their backports when they are as interested in the change.

Agree, ask for the submitter to make backport PR using cherry_picker tool (already installed by requirements/dev.txt) is a good idea.
I think the person who merges a PR should monitor Backport statuses and try to manually backport himself on failure.
It works well pretty often when conflict resolution is trivial.
In a complex case, the merger can kindly ask for manual backporting.

asvetlov · 2020-10-29T09:39:44Z

For the note, backported to 3.8 by e88dc8a

PR aio-libs#5095 by @Teyras Co-authored-by: Sviatoslav Sydorenko <[email protected]> Co-authored-by: Andrew Svetlov <[email protected]>

Add keepalive_timeout parameter to web.run_app

c8ccce0

psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Oct 20, 2020

janbuchar and others added 7 commits October 20, 2020 11:10

Add test for setting keepalive_timeout

fcc2253

Code format

827375f

A more complex way to spy on BaseRunner.__init__

2a99b5c

Update doc

df61102

Update CONTRIBUTORS.txt

dfa3639

Add missing full stop

427489c

Merge branch 'master' into master

7a84d8d

janbuchar marked this pull request as ready for review October 20, 2020 11:25

janbuchar requested review from asvetlov and webknjaz as code owners October 20, 2020 11:25

Fix call_args usage for Python < 3.8

fcc6de6

webknjaz reviewed Oct 20, 2020

View reviewed changes

Update tests/test_run_app.py

998fb71

Co-authored-by: Sviatoslav Sydorenko <[email protected]>

asvetlov approved these changes Oct 21, 2020

View reviewed changes

asvetlov added the backport-3.7 label Oct 21, 2020

asvetlov reviewed Oct 21, 2020

View reviewed changes

docs/web_reference.rst Show resolved Hide resolved

asvetlov assigned webknjaz Oct 21, 2020

asvetlov added backport-3.8 and removed backport-3.7 labels Oct 25, 2020

janbuchar and others added 2 commits October 26, 2020 14:54

Simplify test

f136cd9

Update docs/web_reference.rst

2a33550

Co-authored-by: Andrew Svetlov <[email protected]>

janbuchar requested a review from webknjaz October 26, 2020 13:56

asvetlov reviewed Oct 26, 2020

View reviewed changes

Change versionadded for the keepalive_timeout parameter

4ecfaf8

janbuchar added 2 commits October 27, 2020 19:35

Fix code style

ccefa65

Merge branch 'master' into master

beee1d5

janbuchar requested a review from asvetlov October 27, 2020 18:42

webknjaz reviewed Oct 27, 2020

View reviewed changes

tests/test_run_app.py Outdated Show resolved Hide resolved

webknjaz reviewed Oct 27, 2020

View reviewed changes

tests/test_run_app.py Outdated Show resolved Hide resolved

Apply suggestions from code review

4dda278

Co-authored-by: Sviatoslav Sydorenko <[email protected]>

webknjaz approved these changes Oct 27, 2020

View reviewed changes

webknjaz merged commit 6747ed9 into aio-libs:master Oct 27, 2020

asvetlov added a commit that referenced this pull request Oct 29, 2020

Add keepalive_timeout parameter to web.run_app

e88dc8a

PR #5095 by @Teyras Co-authored-by: Sviatoslav Sydorenko <[email protected]> Co-authored-by: Andrew Svetlov <[email protected]>

commonism pushed a commit to commonism/aiohttp that referenced this pull request Apr 27, 2021

Add keepalive_timeout parameter to web.run_app

e865da9

PR aio-libs#5095 by @Teyras Co-authored-by: Sviatoslav Sydorenko <[email protected]> Co-authored-by: Andrew Svetlov <[email protected]>

commonism pushed a commit to commonism/aiohttp that referenced this pull request Apr 27, 2021

Add keepalive_timeout parameter to web.run_app

7449ec4

PR aio-libs#5095 by @Teyras Co-authored-by: Sviatoslav Sydorenko <[email protected]> Co-authored-by: Andrew Svetlov <[email protected]>

		exc_handler.assert_called_with(patched_loop, msg)


		def test_run_app_keepalive_timeout(patched_loop):

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

Uh oh!

Conversation

janbuchar commented Oct 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What do these changes do?

Are there changes in behavior for the user?

Related issue number

Checklist

Uh oh!

codecov-io commented Oct 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

webknjaz left a comment

Choose a reason for hiding this comment

Uh oh!

webknjaz Oct 20, 2020

Choose a reason for hiding this comment

Uh oh!

janbuchar Oct 20, 2020

Choose a reason for hiding this comment

Uh oh!

webknjaz Oct 20, 2020

Choose a reason for hiding this comment

Uh oh!

janbuchar Oct 20, 2020

Choose a reason for hiding this comment

Uh oh!

webknjaz Oct 20, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

webknjaz Oct 20, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

asvetlov commented Oct 21, 2020

Uh oh!

asvetlov Oct 26, 2020 • edited by webknjaz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janbuchar Oct 27, 2020

Choose a reason for hiding this comment

Uh oh!

janbuchar commented Oct 27, 2020

Uh oh!

asvetlov commented Oct 27, 2020

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Oct 27, 2020

💔 Backport was not successful

Uh oh!

asvetlov commented Oct 29, 2020

Uh oh!

webknjaz commented Oct 29, 2020

Uh oh!

asvetlov commented Oct 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asvetlov commented Oct 29, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

janbuchar commented Oct 20, 2020 •

edited

Loading

codecov-io commented Oct 20, 2020 •

edited

Loading

asvetlov Oct 26, 2020 •

edited by webknjaz

Loading

asvetlov commented Oct 29, 2020 •

edited

Loading