Remove unused readall from Python parser

[Dreamsorcerer]

Here's a test that hasn't been run, ever.

[Dreamsorcerer]

Huh, seems we have a parameter which only exists on the Python parser...

[webknjaz]

I think I saw a few other underscored tests somewhere around the proxy testing modules, IIRC. I don't think I ever got to re-enabling them..

[webknjaz]

I think we should be backporting the test-only changes all the way to the oldest stable we released recently. This helps with future backports, reducing merge conflicts, while not having any effect on the SemVer considerations, because they aren't user-facing. The other day, I spent hours resolving merge conflicts because of the test refactoring from 2019 that hasn't been backported back then but is super useful.

[Dreamsorcerer]

I think I saw a few other underscored tests somewhere around the proxy testing modules, IIRC. I don't think I ever got to re-enabling them..

test_proxy_functional.py has a lot of skips, a third of the file is not run in tests currently.

[webknjaz]

@Dreamsorcerer here's some more: https://github.com/aio-libs/aiohttp/blob/28b574f/tests/test_proxy_functional.py#L461 — just search def xtest in that file to find all 10.

Interestingly, they were disabled in https://github.com/aio-libs/aiohttp/pull/1626#discussion_r101150685 because a maintainer didn't understand why a contributor added them in the first place (https://github.com/aio-libs/aiohttp/pull/1421/files#diff-be15bec475f6c781d5d3e4413c19d0e24ccaf616b8a2495c6824fc3a16d139f5R120). They were then renamed without re-enabling by another maintainer: https://github.com/aio-libs/aiohttp/pull/2256/files#diff-be15bec475f6c781d5d3e4413c19d0e24ccaf616b8a2495c6824fc3a16d139f5R279.

[webknjaz]

@Dreamsorcerer :man_facepalming: I've actually found an issue where I documented this precisely: https://github.com/aio-libs/aiohttp/issues/6031.

[Dreamsorcerer]

@Dreamsorcerer 🤦‍♂️ I've actually found an issue where I documented this precisely: #6031.

Yeah, figured you'd looked at them before. I've no plans to look at the proxy stuff, but fixing the currently failing tests on Windows/Mac with Python 3.12 would be the first priority, before looking at these ones.

[webknjaz]

Fair, I don't think I'll be looking into them anytime soon. But it's good that they are documented.

[Dreamsorcerer]

OK, can someone check this, I think the parameter is totally useless and can be removed. The only place the parameter is used is at https://github.com/aio-libs/aiohttp/blob/9736477ea0387fb9f1e626870894d6cc79bf47c2/aiohttp/http_parser.py#L377

After passing this check: https://github.com/aio-libs/aiohttp/blob/9736477ea0387fb9f1e626870894d6cc79bf47c2/aiohttp/http_parser.py#L359-L363

The value passed through is then used at the end here: https://github.com/aio-libs/aiohttp/blob/9736477ea0387fb9f1e626870894d6cc79bf47c2/aiohttp/http_parser.py#L746-L762

But, if it passed the check above, then it must not be able to reach the code where the variable is used, if I'm reading that right.

[Dreamsorcerer]

If correct, it must then be able to be removed from the latter code too, as the only other code that uses that parameter is hardcoded to True, so anytime that code is reached it must be True.

[Dreamsorcerer]

OK, can someone check this, I think the parameter is totally useless and can be removed.

@bdraco @webknjaz I think these changes are correct, all of that code seems useless and can never happen (except by interacting with HttpPayloadParser directly, as the deleted test was doing).

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 97.62%. Comparing base (0e4a5c3) to head (18c091e).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8096      +/-   ##
==========================================
+ Coverage   97.54%   97.62%   +0.08%     
==========================================
  Files         107      107              
  Lines       33093    33080      -13     
  Branches     3887     3886       -1     
==========================================
+ Hits        32282    32296      +14     
+ Misses        590      567      -23     
+ Partials      221      217       -4     

Flag	Coverage Δ
CI-GHA	97.54% <100.00%> (+0.07%)	:arrow_up:
OS-Linux	97.20% <100.00%> (+0.07%)	:arrow_up:
OS-Windows	95.61% <100.00%> (+0.01%)	:arrow_up:
OS-macOS	96.87% <100.00%> (+0.08%)	:arrow_up:
Py-3.10.11	95.46% <100.00%> (+0.01%)	:arrow_up:
Py-3.10.14	97.01% <100.00%> (+0.01%)	:arrow_up:
Py-3.11.9	97.20% <100.00%> (+0.01%)	:arrow_up:
Py-3.12.3	97.33% <100.00%> (+0.01%)	:arrow_up:
Py-3.8.10	95.39% <100.00%> (+0.01%)	:arrow_up:
Py-3.8.18	96.86% <100.00%> (+0.01%)	:arrow_up:
Py-3.9.13	95.43% <100.00%> (+0.01%)	:arrow_up:
Py-3.9.19	96.99% <100.00%> (+0.04%)	:arrow_up:
Py-pypy7.3.15	96.52% <100.00%> (?)
VM-macos	96.87% <100.00%> (+0.08%)	:arrow_up:
VM-ubuntu	97.20% <100.00%> (+0.07%)	:arrow_up:
VM-windows	95.61% <100.00%> (+0.01%)	:arrow_up:

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[webknjaz]

I'm not sure how I feel about the removal. On one hand, it's an unused private API. And on the other — readall() seem customary for streaming APIs. I suppose, it's fine to drop it and recover later as needed. Though, maybe we'd just make use of it instead, implementing it for Cython too? Thoughts?

[webknjaz]

Another thought: since it's used in some payload tests, they might rely on the underlying buffers being drained automatically or something like that. Could you check? I wouldn't be surprised if this thing exists as a test helper.

[Dreamsorcerer]

readall() seem customary for streaming APIs.

We're removing a parameter called readall, not a method. I think this is also unrelated to streaming APIs, as the parameter is basically just saying "read until EOF when there is no length/chunked specified", which I'm pretty sure is the only sensible (i.e. spec-compliant) approach if the response is not 204 (or similar).

The only place where it ever gets set to False is when we've already determined that that the response has a body (i.e. not 204).

[patchback[bot]]

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

❌ Failed to cleanly apply 175954c010eb2446fe43c3167cdca671a4079e53 on top of patchback/backports/3.10/175954c010eb2446fe43c3167cdca671a4079e53/pr-8096

Backporting merged PR #8096 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.10/175954c010eb2446fe43c3167cdca671a4079e53/pr-8096 upstream/3.10
   

4. Now, cherry-pick PR #8096 contents into that branch:

   $ git cherry-pick -x 175954c010eb2446fe43c3167cdca671a4079e53
   

   If it'll yell at you with something like fatal: Commit 175954c010eb2446fe43c3167cdca671a4079e53 is a merge but no -m option was given., add -m 1 as follows instead:

   $ git cherry-pick -m1 -x 175954c010eb2446fe43c3167cdca671a4079e53
   

5. At this point, you'll probably encounter some merge conflicts. You must resolve them in to preserve the patch from PR #8096 as close to the original as possible.
6. Push this branch to your fork on GitHub:

   $ git push origin patchback/backports/3.10/175954c010eb2446fe43c3167cdca671a4079e53/pr-8096
   

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.