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 #1626 (comment) 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 🤦‍♂️ I've actually found an issue where I documented this precisely: #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

aiohttp/aiohttp/http_parser.py

Line 377 in 9736477

readall=self.readall,

After passing this check:

aiohttp/aiohttp/http_parser.py

Lines 359 to 363 in 9736477

	if not empty_body and (
	(length is not None and length > 0)
	or msg.chunked
	and not msg.upgrade
	):

The value passed through is then used at the end here:

aiohttp/aiohttp/http_parser.py

Lines 746 to 762 in 9736477

	if not response_with_body:
	# don't parse payload if it's not expected to be received
	self._type = ParseState.PARSE_NONE
	real_payload.feed_eof()
	self.done = True
	elif chunked:
	self._type = ParseState.PARSE_CHUNKED
	elif length is not None:
	self._type = ParseState.PARSE_LENGTH
	self._length = length
	if self._length == 0:
	real_payload.feed_eof()
	self.done = True
	else:
	if readall and code != 204:
	self._type = ParseState.PARSE_UNTIL_EOF

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]

Codecov Report

All modified and coverable lines are covered by tests ✅

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%)	⬆️
OS-Linux	97.20% <100.00%> (+0.07%)	⬆️
OS-Windows	95.61% <100.00%> (+0.01%)	⬆️
OS-macOS	96.87% <100.00%> (+0.08%)	⬆️
Py-3.10.11	95.46% <100.00%> (+0.01%)	⬆️
Py-3.10.14	97.01% <100.00%> (+0.01%)	⬆️
Py-3.11.9	97.20% <100.00%> (+0.01%)	⬆️
Py-3.12.3	97.33% <100.00%> (+0.01%)	⬆️
Py-3.8.10	95.39% <100.00%> (+0.01%)	⬆️
Py-3.8.18	96.86% <100.00%> (+0.01%)	⬆️
Py-3.9.13	95.43% <100.00%> (+0.01%)	⬆️
Py-3.9.19	96.99% <100.00%> (+0.04%)	⬆️
Py-pypy7.3.15	96.52% <100.00%> (?)
VM-macos	96.87% <100.00%> (+0.08%)	⬆️
VM-ubuntu	97.20% <100.00%> (+0.07%)	⬆️
VM-windows	95.61% <100.00%> (+0.01%)	⬆️

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

☔ View full report in Codecov by Sentry.
📢 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]

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

❌ Failed to cleanly apply 175954c 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 Remove unused readall from Python parser #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 Remove unused readall from Python parser #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.