Repository navigation
Conversation
…on loss An application that closes its transport from connection_lost(), as aiohttp does, must be left with a transport whose write(), get_write_buffer_size() and set_protocol() behave as after closing a live connection, and loop.sendfile()'s fallback must then raise ConnectionError rather than AttributeError from set_protocol(). The sendfile test reads the file synchronously, so that the fallback is waiting in drain() when the connection is lost; a read in the executor that completes in the same loop iteration as the reset lets the fallback finish first and hides the bug. test_create_connection_memory_leak now keeps the transport past its check, as its comment says it covers, so that it would see an SSLContext kept alive by a transport the application still holds. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…nnection loss
_SSLProtocolTransport.close() dropped its _ssl_protocol when called a second
time, and the transport is already marked closed once the connection is lost,
so an application closing it from connection_lost() left a transport whose
write(), set_protocol(), get_write_buffer_size(), ... raise AttributeError.
That branch was also what released the SSLContext of a transport the
application keeps. Keep the protocol, and release the TLS state from
SSLProtocol.connection_lost() instead: one loop iteration later when the
application's connection_lost() was scheduled, so that it can still read
get_extra_info('ssl_object') and ('sslcontext').
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
All commit authors signed the Contributor License Agreement. |
|
The one failing job, Windows / Build and test (Win32), failed in Could someone with the rights re-run that job? AI disclosure: Claude Opus 5.5 (Anthropic) read the job log and matched it |
|
Thank you for your interest in contributing to Python. This is a community-driven project, and we want it to be a welcoming place to learn and collaborate. Contributors are responsible for reviewing and understanding what they submit, checking its claims, and being able to explain changes in their own words. This helps reviewers focus on the contribution and leaves more time to support other contributors. This contribution appears to have been authored with AI assistance. Please explain your contribution in your own words and describe how you checked its correctness. Please read our guidelines for using AI tools. |
|
@Mariatta Thank you for reviewing my submission. Here's my own-words explanation of the issue and the proposed fix. But first, a disclaimer. I am a regular Python user, as my day job is data analytics and data engineering, and my comfort zone is well downstream from CPython. In other words, it means I know how to leverage the language to solve analytical problems but the inner workings are opaque to me. This issue and proposed fix were discovered on a personal project that's not related to data analytics/engineering -- a project that deals with serving media to clients -- which is on-topic for SSL connection handling. The way I understand the issue is this: when a client unexpectedly drops an SSL connection, the inner workings of CPython are dropping -- i.e. turning into I detailed, with the help of AI, the findings here and was hoping for guidance and confirmation that my findings were, indeed, correct and not hallucinated. However, the evidence AI found seems to point to two important things: the finding appears valid and there's evidence of it happening as far back as Python 3.8 (based on issues raised in other projects); lastly, there's no clean way to fix this without changing a lot of things -- update lots of functions to deal with the As far as I can tell, the proposed solution makes sense to me, but my disclaimer then takes on a load-bearing role: that it makes sense to me does not mean that it is value added for the project. I understand if the mods close this PR due to not meeting AI guidelines -- the same way my initial issue was closed -- as I can compensate for this bug on my end without issues. However, I would much prefer a maintainer with knowledge in this domain take the time to review what the PR says and confirm whether or not it is added value and the finding is correct. This is the heart of collaborative work: communication and discussion. If it turns out to be valid and provides values, it will benefit the community at large and solve edge cases in lots of downstream projects. If not, there's no issue and I won't take it personally. I'm happy to connect and work through any concerns anyone has. |
|
I think a recent issue was opened for that specifically in asyncio: #160964 |
|
@picnixz Thank you for the assist! It is indeed the same issue I'm describing, except there's my evidence points to 3.8+ being affected instead of 3.11+. It's nice to have someone independently arrive at the same conclusion. EDIT: I misremembered the evidence for 3.8+ I had found. The error in those versions was similar in nature but did not stem from the same artifacts. To this end, please ignore the 3.8+ claim above, as the artifacts in question appeared in 3.11+ as stated by other contributors. |
|
@cornopaez Could you resolve merge conflicts? |
Conflicts with pythongh-160513 (connection_lost() no longer skips the app protocol during a start_tls() handshake) in SSLProtocol.connection_lost(), and with pythongh-160499's tests in test_sslproto; both sides kept. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Merge conflicts fixed and a new force push has been sent; just waiting on the CI pipeline to complete as of 10:22am EDT. I appreciate you looking at the proposal, even if you had your own fix lined up. I don't mind which fix goes in as they appear to do the same thing. However, one thing of note: both proposal differ in design but not correctness. Whichever you choose for a merge is OK with me, as the design decision is best left to a regular contributor instead of a newb like me. |
When an SSL connection is lost and the application then closes the transport
(typically from its protocol's
connection_lost(), as aiohttp does),_SSLProtocolTransport.close()took the call for a second close and dropped_ssl_protocol, leaving a transport whosewrite(),set_protocol(),get_write_buffer_size(), ... raiseAttributeError.loop.sendfile()'sfallback meets this in
restore()when the peer goes away mid-transfer.Reported in gh-159875.
This implements the fix proposed in the issue (Proposed fix):
close()no longer drops theprotocol, and the TLS state that branch was releasing (
_sslobj,_sslcontext, and thessl_object/sslcontextextra info) is released bySSLProtocol.connection_lost()instead. It is released one loop iterationlater when the application's
connection_lost()has been scheduled, so thatthe application can still read
get_extra_info('ssl_object')and('sslcontext')there, as it can today.Simply removing the
elsebranch fixes theAttributeErrorbut keeps theSSLContextalive for as long as the application holds the transport; theissue's What the
elsebranch is for and Alternatives consideredcompare the other designs.
Tests
test_sslproto:test_transport_usable_after_connection_lost_and_close,the transport after
connection_lost()thenclose().test_sendfile:test_sendfile_ssl_close_peer_in_the_middle_closes_transport,the fallback with a protocol that closes its transport on connection loss.
It reads the file synchronously so that the fallback is waiting in
drain()when the connection is lost; with the executor read, the raceis lost only occasionally and the test would mostly pass on
main.test_ssl:test_create_connection_memory_leaknow keeps the transportpast its check, which its comment says it covers. With the
elsebranchmerely removed it fails, 3 of 3 runs; on
mainand with this change itpasses.
On macOS (kqueue, poll and select loops), the sendfile test fails in 60 of 60
runs on
mainwithAttributeErrorfromset_protocol(), and passes in 60of 60 with this change.
test_asyncioandtest_sslpass. I have not runit on Windows or Linux; @junaidkhan80121 reported the related
test_asynciocases passing on a Windows build
in the issue.
AI disclosure
Prepared with Claude Opus 5.5 (Anthropic), as in the issue: it wrote the
tests and the change, and ran them on my machine. I reviewed them and am
responsible for them.
🤖 Generated with Claude Code