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.

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

Support setting response header size limits #6559

Merged
merged 8 commits into from Apr 25, 2023

Conversation

jorop
Copy link
Contributor

@jorop jorop commented Jan 25, 2022

What do these changes do?

Allow the user to set the response header limits. The properties max_line_size, max_headers and max_field_size can be set on ClientSession creation. Default values are: max_line_size=8190 max_headers=32768 and max_field_size=8190.
Thus the user can set and - when needed - receive bigger response headers.

Are there changes in behavior for the user?

No

Related issue number

#2304

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

@jorop jorop requested a review from asvetlov as a code owner January 25, 2022 09:19
@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Jan 25, 2022
Copy link
Member

@Dreamsorcerer Dreamsorcerer left a comment

Choose a reason for hiding this comment

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

Will need some tests for the new behaviour to have any chance of being merged, which would also have highlighted the below issue.

aiohttp/client.py Outdated Show resolved Hide resolved
tests/test_client_functional.py Outdated Show resolved Hide resolved
@manuel2295
Copy link

When will this change be approved? This issue is very important to the use of the library.

@Dreamsorcerer
Copy link
Member

There is a test failure, which I've not looked at closely yet. It's not one I've seen before though.

@smonist
Copy link

smonist commented Mar 14, 2022

This is the test that failed: https://github.com/aio-libs/aiohttp/runs/5072020477?check_suite_focus=true#step:10:3041
It looks like the plausible timeout for this test is set too narrow. Can we run the test again and look at the results? Seems not related to the code changes here.

@dmitry-brazhenko
Copy link

Hello @Dreamsorcerer!

Is it possible to merge this PR ? :)

Thanks

CHANGES/2304.feature Outdated Show resolved Hide resolved
aiohttp/client.py Outdated Show resolved Hide resolved
aiohttp/client.py Outdated Show resolved Hide resolved
aiohttp/client.py Outdated Show resolved Hide resolved
aiohttp/client.py Outdated Show resolved Hide resolved
aiohttp/client.py Outdated Show resolved Hide resolved
aiohttp/client.py Outdated Show resolved Hide resolved
aiohttp/client_proto.py Outdated Show resolved Hide resolved
aiohttp/client_proto.py Outdated Show resolved Hide resolved
tests/test_client_functional.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Apr 25, 2023

Codecov Report

Merging #6559 (e8064f9) into master (315ae90) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #6559   +/-   ##
=======================================
  Coverage   97.31%   97.31%           
=======================================
  Files         107      107           
  Lines       31414    31484   +70     
  Branches     3927     3937   +10     
=======================================
+ Hits        30569    30639   +70     
  Misses        641      641           
  Partials      204      204           
Flag Coverage Δ
CI-GHA 97.22% <100.00%> (+<0.01%) ⬆️
OS-Linux 96.88% <100.00%> (+<0.01%) ⬆️
OS-Windows 95.24% <100.00%> (-0.05%) ⬇️
OS-macOS 96.46% <100.00%> (+<0.01%) ⬆️
Py-3.10.11 96.67% <100.00%> (-0.31%) ⬇️
Py-3.11.0 96.41% <100.00%> (+<0.01%) ⬆️
Py-3.7.16 96.70% <100.00%> (+<0.01%) ⬆️
Py-3.7.9 95.17% <100.00%> (+0.01%) ⬆️
Py-3.8.10 95.09% <100.00%> (+0.01%) ⬆️
Py-3.8.16 96.61% <100.00%> (+<0.01%) ⬆️
Py-3.9.13 ?
Py-3.9.16 96.63% <100.00%> (+<0.01%) ⬆️
Py-pypy7.3.11 94.12% <100.00%> (+<0.01%) ⬆️
VM-macos 96.46% <100.00%> (+<0.01%) ⬆️
VM-ubuntu 96.88% <100.00%> (+<0.01%) ⬆️
VM-windows 95.24% <100.00%> (-0.05%) ⬇️

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

Impacted Files Coverage Δ
aiohttp/client_proto.py 96.73% <ø> (ø)
aiohttp/client.py 94.84% <100.00%> (+0.06%) ⬆️
tests/test_client_functional.py 98.51% <100.00%> (+0.04%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Dreamsorcerer Dreamsorcerer added the backport-3.9 Trigger automatic backporting to the 3.9 release branch by Patchback robot label Apr 25, 2023
@Dreamsorcerer Dreamsorcerer merged commit ecd9c72 into aio-libs:master Apr 25, 2023
33 of 35 checks passed
@patchback
Copy link
Contributor

patchback bot commented Apr 25, 2023

Backport to 3.9: 💔 cherry-picking failed — conflicts found

❌ Failed to cleanly apply ecd9c72 on top of patchback/backports/3.9/ecd9c72697ca1529e7cae4693775612987a2a6d2/pr-6559

Backporting merged PR #6559 into master

  1. Ensure you have a local repo clone of your fork. Unless you cloned it
    from the upstream, this would be your origin remote.
  2. Make sure you have an upstream repo added as a remote too. In these
    instructions you'll refer to it by the name upstream. If you don't
    have it, here's how you can add it:
    $ git remote add upstream https://github.com/aio-libs/aiohttp.git
  3. Ensure you have the latest copy of upstream and prepare a branch
    that will hold the backported code:
    $ git fetch upstream
    $ git checkout -b patchback/backports/3.9/ecd9c72697ca1529e7cae4693775612987a2a6d2/pr-6559 upstream/3.9
  4. Now, cherry-pick PR Support setting response header size limits #6559 contents into that branch:
    $ git cherry-pick -x ecd9c72697ca1529e7cae4693775612987a2a6d2
    If it'll yell at you with something like fatal: Commit ecd9c72697ca1529e7cae4693775612987a2a6d2 is a merge but no -m option was given., add -m 1 as follows instead:
    $ git cherry-pick -m1 -x ecd9c72697ca1529e7cae4693775612987a2a6d2
  5. At this point, you'll probably encounter some merge conflicts. You must
    resolve them in to preserve the patch from PR Support setting response header size limits #6559 as close to the
    original as possible.
  6. Push this branch to your fork on GitHub:
    $ git push origin patchback/backports/3.9/ecd9c72697ca1529e7cae4693775612987a2a6d2/pr-6559
  7. Create a PR, ensure that the CI is green. If it's not — update it so that
    the tests and any other checks pass. This is it!
    Now relax and wait for the maintainers to process your pull request
    when they have some cycles to do reviews. Don't worry — they'll tell you if
    any improvements are necessary when the time comes!

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

@Dreamsorcerer
Copy link
Member

If someone could handle the backport to 3.9 as above, that'd be great.

@rohansahai
Copy link
Contributor

Awesome to see this change coming 😃 . Is there any ETA of the 3.9 release?

@Dreamsorcerer
Copy link
Member

Not yet, but still waiting on the backport to 3.9, if you'd like to help out:
#6559 (comment)

rohansahai pushed a commit to rohansahai/aiohttp that referenced this pull request Apr 29, 2023
<!-- Thank you for your contribution! -->

Allow the user to set the response header limits. The properties
max_line_size, max_headers and max_field_size can be set on
ClientSession creation. Default values are: `max_line_size=8190`
`max_headers=32768` and `max_field_size=8190`.
Thus the user can set and - when needed - receive bigger response
headers.

No

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

---------

Co-authored-by: Josef Prenner <josef@cloudomation.com>
Co-authored-by: Sam Bull <aa6bs0@sambull.org>
(cherry picked from commit ecd9c72)
Dreamsorcerer added a commit that referenced this pull request Apr 29, 2023
…559 (#7264)

<!-- Thank you for your contribution! -->

## What do these changes do?
This an attempt at a backport following these guidelines -
#6559 (comment) .
Note, haven't done a backport so no clue if I'm completely missing
something or it was as simple as resolving the merge conflicts when
cherry-picking that work in.

See
#6559 (comment)

---------

Co-authored-by: jorop <jorop@users.noreply.github.com>
Co-authored-by: Sam Bull <aa6bs0@sambull.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-3.9 Trigger automatic backporting to the 3.9 release branch by Patchback robot bot:chronographer:provided There is a change note present in this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants