gh-118950: Fix SSLProtocol.connection_lost not being called when OSError is thrown on asyncio.write. #118960
Conversation
|
All commit authors signed the Contributor License Agreement. |
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
…ocol internal transport closing state so that StreamWriter.drain will invoke sleep(0) which calls connection_lost and correctly notifies waiters of connection lost. (python#118950)
|
As requested @gvanrossum i've opened up a PR for #118950 |
…e connection and cancellation and handle exceptions more gracefully. This addresses the 1006 issue.
|
@kumaraditya303 Do you have time to review this? I know nothing about SSL. Who else could we ask? |
|
Please add some tests, I'll try to find time to review in following week. |
|
I'm not familiar with the SSL code, but the change looks reasonable to me and certainly seems to fix the aiohttp issue (aio-libs/aiohttp#3122). |
|
I will get around to adding some test cases in a Note: All tests are passing, the final check never ran due to GitHub going down. |
|
Is it realistic to expect this bug fix will be backported to 3.12? Or should we expect it in 3.13 or 3.14? |
I did a cursory review of this and read the sslproto.py code. I think this change is correct is should be ok to port to 3.13/12. |
|
As a courtesy, I will keep this PR open a few more days before merging until @kumaraditya303 can take a look at this again. |
Misc/NEWS.d/next/Core and Builtins/2024-05-12-03-10-36.gh-issue-118950.5Wc4vp.rst
Outdated
Show resolved
Hide resolved
…e-118950.5Wc4vp.rst
|
Thanks @cjavad for the PR, and @kumaraditya303 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13. |
…n OSError is thrown (pythonGH-118960) (cherry picked from commit 3f24bde) Co-authored-by: Javad Shafique <[email protected]> Co-authored-by: Kumar Aditya <[email protected]>
…n OSError is thrown (pythonGH-118960) (cherry picked from commit 3f24bde) Co-authored-by: Javad Shafique <[email protected]> Co-authored-by: Kumar Aditya <[email protected]>
|
GH-125931 is a backport of this pull request to the 3.13 branch. |
|
GH-125932 is a backport of this pull request to the 3.12 branch. |
…en OSError is thrown (GH-118960) (#125931) gh-118950: Fix SSLProtocol.connection_lost not being called when OSError is thrown (GH-118960) (cherry picked from commit 3f24bde) Co-authored-by: Javad Shafique <[email protected]> Co-authored-by: Kumar Aditya <[email protected]>
…en OSError is thrown (GH-118960) (#125932) gh-118950: Fix SSLProtocol.connection_lost not being called when OSError is thrown (GH-118960) (cherry picked from commit 3f24bde) Co-authored-by: Javad Shafique <[email protected]> Co-authored-by: Kumar Aditya <[email protected]>
python/cpython#118960 has been fixed which means cleanup closed should no longer be needed
python/cpython#118960 has been fixed an asyncio no longer leaks SSL connections
No longer required due to python/cpython#118960
No longer required due to python/cpython#118960
No longer required due to python/cpython#118960
No longer required due to python/cpython#118960
No longer required due to python/cpython#118960
Let _SSLProtocolTransport.is_closing reflect SSLProtocol internal transport closing state so that StreamWriter.drain will invoke sleep(0) which calls connection_lost and correctly notifies waiters of connection lost. (#118950)
In an existing comment in
asyncio/streams.pythere is some logic that correctly ensures that theconnection_lostfunction is always called for users that keep writing and draining in some other context.The issue here is that this code only runs when the transport is marked as closing, which in the case of the _SSLTransportProtocol only happened after connection_lost has been called.
My proposed fix is for _SSLTransportProtocol.is_closing to also return true when its inner transport is marked as closing, when connection_lost is called the inner transport is removed and instead the _closed flag is set.
No additional tests have been added (yet).
asyncio.sslproto._SSLProtocolTransportcan experience invalid state, leading to silent failures. #118950