GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-142352: Fix start_tls() to transfer buffered data from StreamReader by kasimov-maxim · Pull Request #142354 · python/cpython

Conversation

@kasimov-maxim

@kasimov-maxim kasimov-maxim commented Dec 6, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Fixes #142352

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
fizzi01 and Vizonex reacted with hooray emoji
All reactions
  • 🎉 2 reactions
@kasimov-maxim
kasimov-maxim requested review from 1st1, asvetlov, kumaraditya303 and willingc as code owners December 6, 2025 16:31
@python-cla-bot

python-cla-bot Bot commented Dec 6, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.

CLA signed
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot mentioned this pull request Dec 6, 2025
asyncio: start_tls() loses buffered data from StreamReader #142352
Closed
@bedevere-app bedevere-app Bot added the awaiting review label Dec 6, 2025
kumaraditya303
kumaraditya303 reviewed Dec 17, 2025
View reviewed changes
Comment thread Lib/asyncio/base_events.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@kumaraditya303

kumaraditya303 commented Dec 17, 2025

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@kumaraditya303
kumaraditya303 requested a review from ambv December 17, 2025 16:19
This was referenced Dec 18, 2025
start_tls() loses buffered data and doesn't work when upgrading a stream MagicStack/uvloop#707
Open
Fix start_tls() loses buffer data and hangs when upgrading the connection MagicStack/uvloop#708
Open
start_tls() loses buffered data and doesn't work when upgrading a stream Vizonex/Winloop#102
Closed
Fix start_tls() loses StreamReader buffer data Vizonex/Winloop#103
Merged
picnixz
picnixz previously requested changes Jan 6, 2026
View reviewed changes

@picnixz picnixz left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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/.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_asyncio/test_streams.py Outdated
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.

@picnixz picnixz Jan 6, 2026

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@kasimov-maxim kasimov-maxim Jan 6, 2026

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Understood, will update the tests accordingly.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_asyncio/test_streams.py Outdated
test_message = b"hello world\n"
expected_response = reverse_message(test_message)

class TCPProxyServer:

@picnixz picnixz Jan 6, 2026 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/asyncio/base_events.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot added awaiting changes and removed awaiting review labels Jan 6, 2026
@bedevere-app

bedevere-app Bot commented Jan 6, 2026

Copy link
Copy Markdown

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@picnixz

picnixz commented Jan 6, 2026 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@kasimov-maxim

kasimov-maxim commented Jan 6, 2026

Copy link
Copy Markdown
Contributor Author

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

Noted in PR.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
webknjaz
webknjaz reviewed Jan 16, 2026
View reviewed changes
Comment thread Lib/asyncio/base_events.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
webknjaz
webknjaz reviewed Jan 16, 2026
View reviewed changes
Comment thread Lib/asyncio/base_events.py Outdated
Comment on lines +1335 to +1336
ssl_protocol._incoming.write(buffer)
buffer.clear()

@webknjaz webknjaz Jan 16, 2026

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@kasimov-maxim

kasimov-maxim commented Jan 21, 2026 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

@picnixz

I think reading and checking if we're compliant with https://datatracker.ietf.org/doc/html/rfc2817 should be done

RFC 2817 isn’t relevant here; it covers HTTP/1.1 Upgrade rather than asyncio.start_tls(). At this point we’ve already decided to switch to TLS, so any buffered bytes belong to the TLS layer and should be passed as‑is to the SSL BIO. If non‑TLS bytes are still buffered, that means start_tls() was called too early in the protocol, which is outside asyncio’s responsibility.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@kasimov-maxim
kasimov-maxim force-pushed the gh-142352-start-tls-buffered-data branch from b2ed3c3 to 86d80bf Compare January 21, 2026 16:08
@kasimov-maxim

kasimov-maxim commented Jan 21, 2026

Copy link
Copy Markdown
Contributor Author

I have made the requested changes; please review again
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot added awaiting change review and removed awaiting changes labels Jan 21, 2026
@bedevere-app

bedevere-app Bot commented Jan 21, 2026

Copy link
Copy Markdown

Thanks for making the requested changes!

@picnixz: please review the changes made to this pull request.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app
bedevere-app Bot requested a review from picnixz January 21, 2026 16:15
@kasimov-maxim
kasimov-maxim force-pushed the gh-142352-start-tls-buffered-data branch from 86d80bf to c2cd20b Compare January 21, 2026 16:49
@kasimov-maxim

kasimov-maxim commented Feb 5, 2026

Copy link
Copy Markdown
Contributor Author

@fantix

The root trigger of this bug is, as you stated in the bug report, "the TLS ClientHello (arrived) in the same TCP segment (as some application flag did)".

The new regression test (test_start_tls_buffered_data) shows the trigger is broader than “ClientHello in the same TCP segment as some application flag.” The issue is any buffered TLS data that reaches StreamReader before start_tls() is called. That can happen even without proxy‑protocol or segment coalescing.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@kumaraditya303 kumaraditya303 added topic-asyncio needs backport to 3.13 only security fixes needs backport to 3.14 bugs and security fixes type-bug An unexpected behavior, bug, or error labels Feb 8, 2026
@kumaraditya303

kumaraditya303 commented Feb 8, 2026

Copy link
Copy Markdown
Contributor

Please resolve the Github comments once they are addressed, otherwise it makes reviewing difficult.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
kumaraditya303
kumaraditya303 reviewed Feb 8, 2026
View reviewed changes
Comment thread Lib/test/test_asyncio/test_streams.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
kumaraditya303 added 2 commits February 28, 2026 22:24
@kumaraditya303
address review
d65b052
@kumaraditya303
fix import
dcc279c
@kumaraditya303
kumaraditya303 requested a review from fantix February 28, 2026 17:05
fantix
fantix reviewed Feb 28, 2026
View reviewed changes

@fantix fantix left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
kumaraditya303
kumaraditya303 approved these changes Feb 28, 2026
View reviewed changes
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Feb 28, 2026
@kumaraditya303
kumaraditya303 merged commit 0598f4a into python:main Feb 28, 2026
51 checks passed

Uh oh!

There was an error while loading. Please reload this page.
@miss-islington-app

miss-islington-app Bot commented Feb 28, 2026

Copy link
Copy Markdown

Thanks @kasimov-maxim for the PR, and @kumaraditya303 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14.
🐍🍒⛏🤖
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot removed the awaiting merge label Feb 28, 2026
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Feb 28, 2026
@kasimov-maxim @kumaraditya303 @miss-islington
pythongh-142352: Fix asyncio start_tls() to transfer buffered dat…
def8286 
…a from StreamReader (pythonGH-142354)

(cherry picked from commit 0598f4a)

Co-authored-by: Maksym Kasimov <[email protected]>
Co-authored-by: Kumar Aditya <[email protected]>
@miss-islington-app

miss-islington-app Bot commented Feb 28, 2026

Copy link
Copy Markdown

Sorry, @kasimov-maxim and @kumaraditya303, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 0598f4a8999b96409e0a2bf9c480afc76a876860 3.13
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@miss-islington-app miss-islington-app Bot assigned kumaraditya303 Feb 28, 2026
@bedevere-app

bedevere-app Bot commented Feb 28, 2026

Copy link
Copy Markdown

GH-145363 is a backport of this pull request to the 3.14 branch.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot removed the needs backport to 3.14 bugs and security fixes label Feb 28, 2026
@bedevere-app

bedevere-app Bot commented Feb 28, 2026

Copy link
Copy Markdown

GH-145364 is a backport of this pull request to the 3.13 branch.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot removed the needs backport to 3.13 only security fixes label Feb 28, 2026
kumaraditya303 added a commit to kumaraditya303/cpython that referenced this pull request Feb 28, 2026
@kumaraditya303 @kasimov-maxim
[3.13] pythongh-142352: Fix asyncio start_tls() to transfer buffe…
9b79465 
…red data from StreamReader (pythonGH-142354)

(cherry picked from commit 0598f4a)

Co-authored-by: Maksym Kasimov <[email protected]>
Co-authored-by: Kumar Aditya <[email protected]>
kumaraditya303 added a commit to miss-islington/cpython that referenced this pull request Feb 28, 2026
@kumaraditya303 @kasimov-maxim
pythongh-142352: Fix asyncio start_tls() to transfer buffered dat…
5fc071b 
…a from StreamReader (pythonGH-142354)

(cherry picked from commit 0598f4a)

Co-authored-by: Maksym Kasimov <[email protected]>
Co-authored-by: Kumar Aditya <[email protected]>
brijkapadia pushed a commit to brijkapadia/cpython that referenced this pull request Feb 28, 2026
@kasimov-maxim @kumaraditya303 @brijkapadia
pythongh-142352: Fix asyncio start_tls() to transfer buffered dat…
78a274c 
…a from StreamReader (python#142354)

Co-authored-by: Kumar Aditya <[email protected]>
kumaraditya303 added a commit that referenced this pull request Feb 28, 2026
@miss-islington @kumaraditya303 @kasimov-maxim
[3.14] gh-142352: Fix asyncio start_tls() to transfer buffered da…
fc2a6cf 
…ta from StreamReader (GH-142354) (#145363)

gh-142352: Fix `asyncio` `start_tls()` to transfer buffered data from StreamReader (GH-142354)
(cherry picked from commit 0598f4a)

Co-authored-by: Kumar Aditya <[email protected]>
Co-authored-by: Maksym Kasimov <[email protected]>
kumaraditya303 added a commit that referenced this pull request Feb 28, 2026
@kumaraditya303 @kasimov-maxim
[3.13] gh-142352: Fix asyncio start_tls() to transfer buffered data f…
aa0aa31 
…rom StreamReader (GH-142354) (#145364)

[3.13] gh-142352: Fix `asyncio` `start_tls()` to transfer buffered data from StreamReader (GH-142354)
(cherry picked from commit 0598f4a)

Co-authored-by: Maksym Kasimov <[email protected]>
ljfp pushed a commit to ljfp/cpython that referenced this pull request Apr 25, 2026
@kasimov-maxim @kumaraditya303 @ljfp
pythongh-142352: Fix asyncio start_tls() to transfer buffered dat…
4332dd0 
…a from StreamReader (python#142354)

Co-authored-by: Kumar Aditya <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@webknjaz webknjaz webknjaz left review comments
@kumaraditya303 kumaraditya303 kumaraditya303 approved these changes
@1st1 1st1 Awaiting requested review from 1st1 1st1 is a code owner
@asvetlov asvetlov Awaiting requested review from asvetlov asvetlov is a code owner
@willingc willingc Awaiting requested review from willingc willingc is a code owner
@ambv ambv Awaiting requested review from ambv
@picnixz picnixz Awaiting requested review from picnixz
+1 more reviewer
@fantix fantix fantix left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@kumaraditya303 kumaraditya303

Labels

topic-asyncio type-bug An unexpected behavior, bug, or error

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

asyncio: start_tls() loses buffered data from StreamReader

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@kasimov-maxim @kumaraditya303 @picnixz @webknjaz @fantix
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.