Stream unpauses protocol before releasing connection #10171

javitonino · 2024-12-17T12:13:35Z

What do these changes do?

Fixes a hang when reusing a connection that was previously used for a streaming download and returned to the pool in paused state.

Are there changes in behavior for the user?

No, aside from the bugfix.

Is it a substantial burden for the maintainers to support this?

No. This adds bit of extra code to ensure correctness in this case. It's covered by tests to ensure the behaviour stays through code changes.

Related issue number

Fixes #10169

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

…10169)

codecov · 2024-12-17T12:22:05Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.75%. Comparing base (0070045) to head (b5dc127).
Report is 4 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #10171   +/-   ##
=======================================
  Coverage   98.75%   98.75%           
=======================================
  Files         122      122           
  Lines       36976    36995   +19     
  Branches     4412     4413    +1     
=======================================
+ Hits        36517    36536   +19     
  Misses        313      313           
  Partials      146      146

Flag	Coverage Δ
CI-GHA	`98.64% <100.00%> (+<0.01%)`	⬆️
OS-Linux	`98.33% <100.00%> (+<0.01%)`	⬆️
OS-Windows	`96.18% <100.00%> (+<0.01%)`	⬆️
OS-macOS	`97.44% <100.00%> (+<0.01%)`	⬆️
Py-3.10.11	`97.28% <100.00%> (+<0.01%)`	⬆️
Py-3.10.15	`97.87% <100.00%> (+<0.01%)`	⬆️
Py-3.11.10	`?`
Py-3.11.11	`97.86% <100.00%> (-0.07%)`	⬇️
Py-3.11.9	`97.33% <100.00%> (-0.01%)`	⬇️
Py-3.12.7	`?`
Py-3.12.8	`98.38% <100.00%> (+<0.01%)`	⬆️
Py-3.13.0	`?`
Py-3.13.1	`98.33% <100.00%> (-0.06%)`	⬇️
Py-3.9.13	`97.20% <100.00%> (+<0.01%)`	⬆️
Py-3.9.20	`97.74% <100.00%> (-0.05%)`	⬇️
Py-pypy7.3.16	`97.36% <100.00%> (+<0.01%)`	⬆️
VM-macos	`97.44% <100.00%> (+<0.01%)`	⬆️
VM-ubuntu	`98.33% <100.00%> (+<0.01%)`	⬆️
VM-windows	`96.18% <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.

codspeed-hq · 2024-12-17T12:22:31Z

CodSpeed Performance Report

Merging #10171 will not alter performance

_{Comparing javitonino:bugfix_10169 (b5dc127) with master (0070045)}

Summary

✅ 47 untouched benchmarks

Dreamsorcerer · 2024-12-17T13:25:47Z

I'm wondering if doing it in BaseConnector._release() may be the safest option, to ensure that it is always unpaused regardless of what any other components might do. Anybody else have any thoughts on it?

asvetlov · 2024-12-17T13:33:49Z

Please give me some time for the review.

javitonino · 2024-12-17T13:36:28Z

I think it makes sense to have this, at least, in the StreamReader, in the sense of having it clean up after itself.

However, I think that having a global catch-all at connection release is also a good idea, just in case we missed some other case that can end up in the same situation. I only did this change because I'm not sure of the consequences of doing this at a more general point might have (looks like it should be ok, but I'm just a newcomer here).

Dreamsorcerer · 2024-12-17T13:43:44Z

I only did this change because I'm not sure of the consequences of doing this at a more general point might have (looks like it should be ok, but I'm just a newcomer here).

Yeah, I just figure that if we're releasing a connection, then it has no business being paused. Anything that was trying to read from it has already finished, we're now moving onto parsing a whole new HTTP message.

bdraco · 2024-12-17T18:11:05Z

I'm assuming this happens on the last chunk of data for the payload which it just enough to go over the high water mark, and than the parser is complete and calls feed_eof. I can't see how else it could happen. I don't see an attack vector here that could be used to fill up memory since the next feed_data call would pause again anyways (not that we should get one after eof).

This solution makes sense to me. We should probably do a release sooner rather than later to get this fix out there since its this type of reliability issue is so hard to track down, and its very likely to be happening in other real world use cases.

aiohttp/streams.py

asvetlov

Looks good, let's merge and publish a bugfix release

bdraco · 2024-12-18T17:12:45Z

If I don't run out of time, I'll do a release later today (utc-10)

patchback · 2024-12-18T18:28:04Z

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

❌ Failed to cleanly apply 5185f93 on top of patchback/backports/3.11/5185f932407947fbbabd31c5dc6d72abc5c54171/pr-10171

Backporting merged PR #10171 into master

Ensure you have a local repo clone of your fork. Unless you cloned it
from the upstream, this would be your origin remote.
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
```

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.11/5185f932407947fbbabd31c5dc6d72abc5c54171/pr-10171 upstream/3.11

Now, cherry-pick PR Stream unpauses protocol before releasing connection #10171 contents into that branch:
```
$ git cherry-pick -x 5185f932407947fbbabd31c5dc6d72abc5c54171
```
If it'll yell at you with something like fatal: Commit 5185f932407947fbbabd31c5dc6d72abc5c54171 is a merge but no -m option was given., add -m 1 as follows instead:
```
$ git cherry-pick -m1 -x 5185f932407947fbbabd31c5dc6d72abc5c54171
```
At this point, you'll probably encounter some merge conflicts. You must
resolve them in to preserve the patch from PR Stream unpauses protocol before releasing connection #10171 as close to the
original as possible.

Push this branch to your fork on GitHub:

$ git push origin patchback/backports/3.11/5185f932407947fbbabd31c5dc6d72abc5c54171/pr-10171

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.

patchback · 2024-12-18T18:28:11Z

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

❌ Failed to cleanly apply 5185f93 on top of patchback/backports/3.12/5185f932407947fbbabd31c5dc6d72abc5c54171/pr-10171

Backporting merged PR #10171 into master

Ensure you have a local repo clone of your fork. Unless you cloned it
from the upstream, this would be your origin remote.
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
```

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.12/5185f932407947fbbabd31c5dc6d72abc5c54171/pr-10171 upstream/3.12

Now, cherry-pick PR Stream unpauses protocol before releasing connection #10171 contents into that branch:
```
$ git cherry-pick -x 5185f932407947fbbabd31c5dc6d72abc5c54171
```
If it'll yell at you with something like fatal: Commit 5185f932407947fbbabd31c5dc6d72abc5c54171 is a merge but no -m option was given., add -m 1 as follows instead:
```
$ git cherry-pick -m1 -x 5185f932407947fbbabd31c5dc6d72abc5c54171
```
At this point, you'll probably encounter some merge conflicts. You must
resolve them in to preserve the patch from PR Stream unpauses protocol before releasing connection #10171 as close to the
original as possible.

Push this branch to your fork on GitHub:

$ git push origin patchback/backports/3.12/5185f932407947fbbabd31c5dc6d72abc5c54171/pr-10171

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.

(cherry picked from commit 5185f93)

bdraco · 2024-12-18T18:42:44Z

3.11 backport #10179

(cherry picked from commit 5185f93)

bdraco · 2024-12-18T18:43:40Z

3.12 backport #10180

…leasing connection (#10179) Co-authored-by: Javier Torres <javier@javiertorres.eu>

…leasing connection (#10180) Co-authored-by: Javier Torres <javier@javiertorres.eu>

bdraco · 2024-12-18T19:23:17Z

Thanks @javitonino

Working on release 3.11.11 now

#10181

bdraco · 2024-12-18T22:08:54Z

3.11.11 is now available with this fix

Stream unpauses protocol before releasing connection (fixes aio-libs#…

1deea14

…10169)

javitonino requested review from webknjaz and asvetlov as code owners December 17, 2024 12:13

psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Dec 17, 2024

mypy annotations

b5dc127

javitonino mentioned this pull request Dec 17, 2024

aiohttp hangs when uploading a file during streaming download #10169

Closed

1 task

Dreamsorcerer added backport-3.11 Trigger automatic backporting to the 3.11 release branch by Patchback robot backport-3.12 Trigger automatic backporting to the 3.12 release branch by Patchback robot labels Dec 17, 2024

bdraco approved these changes Dec 17, 2024

View reviewed changes

bdraco reviewed Dec 17, 2024

View reviewed changes

aiohttp/streams.py Show resolved Hide resolved

bdraco changed the title ~~Stream unpauses protocol before releasing connection (fixes #10169)~~ Stream unpauses protocol before releasing connection Dec 17, 2024

asvetlov approved these changes Dec 18, 2024

View reviewed changes

bdraco merged commit 5185f93 into aio-libs:master Dec 18, 2024
41 of 42 checks passed

bdraco pushed a commit that referenced this pull request Dec 18, 2024

Stream unpauses protocol before releasing connection (#10171)

9bd7ad8

(cherry picked from commit 5185f93)

bdraco pushed a commit that referenced this pull request Dec 18, 2024

Stream unpauses protocol before releasing connection (#10171)

2dbde4c

(cherry picked from commit 5185f93)

bdraco added a commit that referenced this pull request Dec 18, 2024

[PR #10171/5185f93 backport][3.11] Stream unpauses protocol before re…

db56d74

…leasing connection (#10179) Co-authored-by: Javier Torres <javier@javiertorres.eu>

bdraco added a commit that referenced this pull request Dec 18, 2024

[PR #10171/5185f93 backport][3.12] Stream unpauses protocol before re…

a3a5716

…leasing connection (#10180) Co-authored-by: Javier Torres <javier@javiertorres.eu>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stream unpauses protocol before releasing connection #10171

Stream unpauses protocol before releasing connection #10171

javitonino commented Dec 17, 2024 •

edited

Loading

codecov bot commented Dec 17, 2024 •

edited

Loading

codspeed-hq bot commented Dec 17, 2024

Dreamsorcerer commented Dec 17, 2024

asvetlov commented Dec 17, 2024

javitonino commented Dec 17, 2024

Dreamsorcerer commented Dec 17, 2024

bdraco commented Dec 17, 2024 •

edited

Loading

asvetlov left a comment

bdraco commented Dec 18, 2024 •

edited

Loading

patchback bot commented Dec 18, 2024 •

edited

Loading

patchback bot commented Dec 18, 2024 •

edited

Loading

bdraco commented Dec 18, 2024

bdraco commented Dec 18, 2024

bdraco commented Dec 18, 2024

bdraco commented Dec 18, 2024

Stream unpauses protocol before releasing connection #10171

Stream unpauses protocol before releasing connection #10171

Conversation

javitonino commented Dec 17, 2024 • edited Loading

What do these changes do?

Are there changes in behavior for the user?

Is it a substantial burden for the maintainers to support this?

Related issue number

Checklist

codecov bot commented Dec 17, 2024 • edited Loading

Codecov Report

codspeed-hq bot commented Dec 17, 2024

CodSpeed Performance Report

Merging #10171 will not alter performance

Summary

Dreamsorcerer commented Dec 17, 2024

asvetlov commented Dec 17, 2024

javitonino commented Dec 17, 2024

Dreamsorcerer commented Dec 17, 2024

bdraco commented Dec 17, 2024 • edited Loading

asvetlov left a comment

Choose a reason for hiding this comment

bdraco commented Dec 18, 2024 • edited Loading

patchback bot commented Dec 18, 2024 • edited Loading

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

Backporting merged PR #10171 into master

patchback bot commented Dec 18, 2024 • edited Loading

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

Backporting merged PR #10171 into master

bdraco commented Dec 18, 2024

bdraco commented Dec 18, 2024

bdraco commented Dec 18, 2024

bdraco commented Dec 18, 2024

javitonino commented Dec 17, 2024 •

edited

Loading

codecov bot commented Dec 17, 2024 •

edited

Loading

bdraco commented Dec 17, 2024 •

edited

Loading

bdraco commented Dec 18, 2024 •

edited

Loading

patchback bot commented Dec 18, 2024 •

edited

Loading

patchback bot commented Dec 18, 2024 •

edited

Loading