Skip to content

gh-109051: asyncio: remove outgoing buffer from sslproto #109571

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

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

sorcio
Copy link
Contributor

@sorcio sorcio commented Sep 19, 2023

asyncio/sslproto.py TLS wrapper has a redundant outgoing buffer (the MemoryBIO object already serves as a buffer), and a flow control layer for outgoing data which serves an unclear purpose

If this extra logic can be removed, it would solve the AssertionError issue described in the issue as a secondary effect, because the asserted condition cannot be guaranteed in general (details in issue comments).

For reference, the current sslproto.py code was ported from uvloop, which manifests the same issue. The uvloop implementation uses the MemoryBIO directly as a write buffer, but still has the extra flow control logic which causes the mismatch.

@sorcio
Copy link
Contributor Author

sorcio commented Sep 19, 2023

This breaks so many tests that I didn't see locally. Feel free to downgrade to draft or close.

@sorcio
Copy link
Contributor Author

sorcio commented Sep 19, 2023

Possibly fixed what was breaking. But I'm less confident of the validity of the change because I had to touch sendfile stuff.

My stubborn idea was that _FlowControlMixin should not be necessary (both in sendfile, which explicitly checks for it, and in start_tls, which could check for it to fix the issue but doesn't). Not depending on internals makes it easier to add wrapper transports/protocols, including the TLS wrapper itself. But there could be some other good reason that I don't see. Anyway, this is open for discussion.


The alternative fix is much easier (but feels dirtier to me). Something like: in start_tls, report an error if the transport is not a _FlowControlMixin, and then pass down the value of _protocol_paused to SSLProtocol.

Even in this case, the redundant "outgoing" buffer in the wrapper could be removed (again, unless I'm missing something important), but it's an orthogonal issue.

@gvanrossum
Copy link
Member

You can make it draft yourself. Maybe @vstinner can help?

@sorcio
Copy link
Contributor Author

sorcio commented Sep 25, 2023

I don't have the option to convert to draft (maybe it's not available to external contributors).

Anyway, the PR is open for review now.

The only thing is that this is not just a fix to gh-109051 but a kind-of deeper change. That's why I suggested #109603 as an immediate fix.

@vstinner
Copy link
Member

cc @1st1

@gvanrossum gvanrossum marked this pull request as draft September 25, 2023 19:33
@kumaraditya303 kumaraditya303 removed their request for review March 14, 2025 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants