Skip to content

gh-127655: Ensure _SelectorSocketTransport.writelines pauses the protocol if needed #127656

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.

Sign up for GitHub

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

Jump to bottom
Merged
kumaraditya303 merged 10 commits into from
Dec 6, 2024
Merged

gh-127655: Ensure _SelectorSocketTransport.writelines pauses the protocol if needed #127656

kumaraditya303 merged 10 commits into from
Dec 6, 2024
+14 −0

Conversation

@bdraco
Copy link
Contributor

@bdraco bdraco commented Dec 5, 2024 •
edited by picnixz
Loading

gh-127655: Ensure writelines pauses the protocol if needed

Note for reviewers: 3.12+ only since writelines only exists since 3.12.

@bedevere-app bedevere-app bot mentioned this pull request Dec 5, 2024
Closed
@bdraco bdraco changed the title gh-127655: Ensure writelines pauses the protocol if needed gh-127655: Ensure _SelectorSocketTransport.writelines pauses the protocol if needed Dec 5, 2024
@bdraco
Copy link
Contributor Author

bdraco commented Dec 5, 2024

Looks like the CI failure was GitHub being flakey and not an actual problem AFAICT

@bdraco bdraco mentioned this pull request Dec 5, 2024
Merged
This was referenced Dec 5, 2024
Merged
Merged
@bdraco bdraco mentioned this pull request Dec 5, 2024
Merged
@gvanrossum
Copy link
Member

gvanrossum commented Dec 5, 2024 via email

@picnixz picnixz added needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes labels Dec 6, 2024
gvanrossum
gvanrossum approved these changes Dec 6, 2024
View reviewed changes
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

The one-line fix looks good.

The test doesn't really test anything except that the new logic is in place -- it doesn't prove at all that it does what it claims to do (prevent memory filling up if the other end never reads). But that's hard to test without bringing the test machine to its knees, so as long as you have tested this on one of your actual reproducing scenarios, I'm okay with this.

I suppose some people will be surprised that they are now paused by voluminous writelines() calls where previously they weren't. (What happens if you are paused but you keep calling writelines() anyway?) But consistency with write() is more important, and that may already pause. So I'm okay with this, since writelines() is supposed to be equivalent to multiple write() calls.

@bdraco
Copy link
Contributor Author

bdraco commented Dec 6, 2024

New preview
https://cpython-previews--127656.org.readthedocs.build/en/127656/whatsnew/changelog.html#changelog
Screenshot 2024-12-05 at 10 24 40 PM

@kumaraditya303
Copy link
Contributor

kumaraditya303 commented Dec 6, 2024

If the protocol is paused, the example in the PoC won't call writelines anymore until the buffer is drained.

Yeah but if you still continue to call writelines then I guess the memory will go unbounded right?

@kumaraditya303
Copy link
Contributor

kumaraditya303 commented Dec 6, 2024

@python/organization-owners please add me so I can view the advisory

@bdraco
Copy link
Contributor Author

bdraco commented Dec 6, 2024 •
edited
Loading

If the protocol is paused, the example in the PoC won't call writelines anymore until the buffer is drained.

Yeah but if you still continue to call writelines then I guess the memory will go unbounded right?

Yes if the implementation doesn't drain or otherwise wait to send more when the protocol is paused, the same is true about write, but thats a downstream implementation problem.

@kumaraditya303 kumaraditya303 merged commit e991ac8 into python:main Dec 6, 2024
38 checks passed
@miss-islington-app
Copy link

miss-islington-app bot commented Dec 6, 2024

Thanks @bdraco for the PR, and @kumaraditya303 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

@bedevere-app bedevere-app bot removed the