gh-142352: Fix start_tls() to transfer buffered data from StreamReader #142354

kasimov-maxim · 2025-12-06T16:31:44Z

Summary

When using StreamWriter.start_tls() to upgrade a connection to TLS mid-stream, any data already buffered in the StreamReader was lost. This commonly occurs when implementing servers that support the PROXY protocol.

Changes

Added _transfer_buffered_data_to_ssl() method to BaseEventLoop that transfers buffered data from StreamReader._buffer to SSLProtocol._incoming before the TLS handshake begins
Added tests for the PROXY protocol + TLS scenario

Generative AI usage

Some parts of this PR were assisted by a generative AI tool. Specifically:

help with drafting English-language comments and documentation
suggestions for test refactoring
assistance with wording and structuring the PR description

All functional logic and final edits were reviewed, verified, and authored by
me, in accordance with the Python devguide guidelines.

Test plan

Added new tests in test_streams.py
All asyncio tests pass
make patchcheck passes

Issue: asyncio: start_tls() loses buffered data from StreamReader #142352

python-cla-bot · 2025-12-06T16:31:49Z

All commit authors signed the Contributor License Agreement.

kumaraditya303 · 2025-12-17T16:15:30Z

Lib/asyncio/base_events.py

                file.seek(offset + total_sent)
            await proto.restore()

+    def _transfer_buffered_data_to_ssl(self, protocol, ssl_protocol):


As this method is called from just one place, I suggest to inline it

I kept this logic in a small helper because it represents a distinct
conceptual step (bridging StreamReader buffering into the SSLProtocol BIO),
and start_tls() is already quite dense. Even though the block is relatively
short, it introduces non-obvious buffering semantics involving private
StreamReader and SSLProtocol internals.

Keeping it named helps document the behavior required for mid-connection
start_tls() scenarios, avoids cluttering the main flow, and leaves room
for future adjustments if this logic needs to grow.

If we inline this, I think it would be worth keeping a fairly detailed
comment explaining why this buffering transfer is needed.

That said, I'm happy to inline it if you think that's preferable here.

There is no need for a method as it would indirectly be exposed on subclasses. The docstring is also un-necessary, and there is no need to mention custom implementations.

If we inline this, I think it would be worth keeping a fairly detailed
comment explaining why this buffering transfer is needed.

No. Just link an issue (and possibly a short comment). The code base prefers concise comments. It's not meant to be read by everyone and we definitely don't want to have more docstrings than code. If something is meant to be extensively documented it should be done at the RST level.

Understood, thanks. I'll inline it and keep only a short comment with an issue reference.

(my $0.02)

no need for a method as it would indirectly be exposed on subclasses

FWIW, I often make use of the technique of naming things. It's nice to have clear semantic names for such abstractions: it'd be more difficult to make sense of ssl_protocol._incoming.write(protocol._stream_reader._buffer) in the context of start_tls(). Plus having these low-level details leak into that layer would mean that future refactorings may result in unrelated lines being added in-between, making their way into a block of otherwise tightly-coupled instructions.

Clearly, having it a member of the class is suboptimal but what's wrong with having a _private helper function around, for example?

We do not like proliferation of methods when code can be inlined. Making sense of line is the reason why adding a small comment is enough.

We also should not be worried about the future. However I would recommend having a more compact block (we avoid blank lines in the stdlib and only use them to separate sections in a function).

Now, If we want to make it private, why not also. But I do not think that it is better to leak the method itself to subclasses (because then we need to indicate that the logic is private and neither protected or public; this the unfortunate part of not having a protected convention aside from @override).

Since Kumar is the code owner I think we should respect his intentions on that matter. Though they could have changed after your suggestion.

kumaraditya303 · 2025-12-17T16:18:57Z

@ambv Can you review this? You reviewed the original PR which added this API #91453

picnixz

Has this PR been generated using an LLM? if so, indicate which places were so and read https://devguide.python.org/getting-started/generative-ai/.

picnixz · 2026-01-06T12:53:09Z

Lib/test/test_asyncio/test_streams.py

        self.assertEqual(msg2, b"hello world 2!\n")

+    def _run_test_start_tls_behind_proxy(self, send_combined):
+        """Test start_tls() when TLS ClientHello arrives with PROXY header.


Remove docstrings from tests. It would be shown on failure. Use regular comments and shorten it. We don't want to restate the entire issue so simply link the issue's number as well.

Understood, will update the tests accordingly.

picnixz · 2026-01-06T12:53:26Z

Lib/test/test_asyncio/test_streams.py

+        test_message = b"hello world\n"
+        expected_response = reverse_message(test_message)
+
+        class TCPProxyServer:


Do we really need this class? don't we already have some helpers elsewhere? The test also looks overly complex. Why not just patching existing TCPServer? (if possible; I don't know if there isn't some class that could be used as is with some minimal modifications)

picnixz · 2026-01-06T12:58:18Z

Lib/asyncio/base_events.py

                file.seek(offset + total_sent)
            await proto.restore()

+    def _transfer_buffered_data_to_ssl(self, protocol, ssl_protocol):


There is no need for a method as it would indirectly be exposed on subclasses. The docstring is also un-necessary, and there is no need to mention custom implementations.

If we inline this, I think it would be worth keeping a fairly detailed
comment explaining why this buffering transfer is needed.

No. Just link an issue (and possibly a short comment). The code base prefers concise comments. It's not meant to be read by everyone and we definitely don't want to have more docstrings than code. If something is meant to be extensively documented it should be done at the RST level.

bedevere-app · 2026-01-06T13:00:48Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

picnixz · 2026-01-06T13:07:13Z

Also, I think reading and checking if we're compliant with https://datatracker.ietf.org/doc/html/rfc2817 should be done (even if the RFC is only proposed and not in the standard tracks) or whatever RFC should be considered (I do think it's correct to fix this but I don't know if we should only send the possible hello for tls or generally write whatever was buffered)

kasimov-maxim · 2026-01-06T15:37:39Z

Has this PR been generated using an LLM? if so, indicate which places were so

Noted in PR.

webknjaz · 2026-01-16T15:52:26Z

Lib/asyncio/base_events.py

+
        # Pause early so that "ssl_protocol.data_received()" doesn't
        # have a chance to get called before "ssl_protocol.connection_made()".
        transport.pause_reading()


Wouldn't it make sense to transfer the buffer after reading is paused rather than before?

webknjaz · 2026-01-16T16:45:31Z

Lib/asyncio/base_events.py

+            ssl_protocol._incoming.write(buffer)
+            buffer.clear()


I wonder if this could better belong @

cpython/Lib/asyncio/sslproto.py

Line 322 in b2ed3c3

self._incoming = ssl.MemoryBIO()

.

Technically, there could be something like

- self._incoming = ssl.MemoryBIO() + self._incoming = ssl.MemoryBIO() + try: + underlying_raw_stream_buffer = app_protocol._stream_reader._buffer + except AttributeError: + pass + else: + self._incoming.write(underlying_raw_stream_buffer) + underlying_raw_stream_buffer.clear()

But I don't know how other places that initialize sslproto.SSLProtocol() would be affected. So it's just an idea to check out and see if other instances should have the same buffer transfer logic (looks like it's used in two other modules).

asyncio.streams: transfer buffered data to SSL layer in start_tls()

b2ed3c3

kasimov-maxim requested review from 1st1, asvetlov, kumaraditya303 and willingc as code owners December 6, 2025 16:31

bedevere-app bot mentioned this pull request Dec 6, 2025

asyncio: start_tls() loses buffered data from StreamReader #142352

Open

bedevere-app bot added the awaiting review label Dec 6, 2025

kumaraditya303 reviewed Dec 17, 2025

View reviewed changes

kumaraditya303 requested a review from ambv December 17, 2025 16:19

picnixz requested changes Jan 6, 2026

View reviewed changes

bedevere-app bot added awaiting changes and removed awaiting review labels Jan 6, 2026

webknjaz reviewed Jan 16, 2026

View reviewed changes

Uh oh!

gh-142352: Fix start_tls() to transfer buffered data from StreamReader #142354

Are you sure you want to change the base?

gh-142352: Fix start_tls() to transfer buffered data from StreamReader #142354

Conversation

kasimov-maxim commented Dec 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Generative AI usage

Test plan

Uh oh!

python-cla-bot bot commented Dec 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

picnixz Jan 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

picnixz Jan 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kumaraditya303 commented Dec 17, 2025

Uh oh!

picnixz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

picnixz Jan 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

picnixz Jan 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-app bot commented Jan 6, 2026

Uh oh!

picnixz commented Jan 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kasimov-maxim commented Jan 6, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kasimov-maxim commented Dec 6, 2025 •

edited

Loading

python-cla-bot bot commented Dec 6, 2025 •

edited

Loading

picnixz Jan 6, 2026 •

edited

Loading

picnixz Jan 16, 2026 •

edited

Loading

picnixz Jan 6, 2026 •

edited

Loading

picnixz Jan 6, 2026 •

edited

Loading

picnixz commented Jan 6, 2026 •

edited

Loading