gh-137583: Only lock the SSL context, not the SSL socket #137588
Conversation
|
!buildbot ^((?!refleak).)* PR$ |
This comment was marked as outdated.
This comment was marked as outdated.
|
(You'll get failures due to the buildbots without any built-in hashes and with FIPS mode on) |
I spent all that time trying to make sure the test worked and of course it was the blurb that was wrong too.
|
The blurb was ok. We just don't document those methods because I think they are actually available simply as |
|
That's frustrating. I can't reproduce it emitting warnings locally, so I thought d45f6a8 fixed it. |
|
Oh no, actually, recv() doesn't exist on SSLSockets. It's socket.socket.recv(). Same for send(). If you want you can do: :meth:`SSLSocket.recv <socket.socket.recv>` |
|
We'll deal with the blurb later, I avoided the reference altogether now. There's an actual test failure here: https://github.com/python/cpython/actions/runs/16849797228/job/47734333301 I think this is that stupid race condition. I'll look into fixing |
Apparently, the thread can start waiting at a point where the server isn't ready to receive messages, which caused it to block indefinitely for some reason. Before we create any threads or set any events, do a simple exchange with the server to ensure that it's in its message loop. This took way too damn long to figure out.
|
There we go, that race is finally fixed. Let's try the buildbots again. |
|
🤖 New build scheduled with the buildbot fleet by @ZeroIntensity for commit 38e29f9 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F137588%2Fmerge If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
| @@ -0,0 +1,4 @@ | |||
| Fix a deadlock introduced in the :mod:`ssl` module when a call to | |||
There was a problem hiding this comment.
mention the specific Python 3.13 version the deadlock was introduced in.
There was a problem hiding this comment.
The backport didn't make it to 3.14, so it's fine there. Only 3.13.6 is affected.
Apparently, settimeout() implicitly makes the socket non-blocking.
|
!buildbot windows |
|
🤖 New build scheduled with the buildbot fleet by @ZeroIntensity for commit 6da074e 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F137588%2Fmerge The command will test the builders whose names match following regular expression: The builders matched are:
|
|
@picnixz Did you want a chance to review this, or are you fine with me merging it? |
|
Go ahead, I don't have a lot of time today and I'm leaving tomorrow for 10 days |
| Fix a deadlock introduced in 3.13.6 when a call to | ||
| :meth:`ssl.SSLSocket.recv <socket.socket.recv>` was blocked in one thread, while another | ||
| attempted to call :meth:`ssl.SSLSocket.send <socket.socket.send>` to unblock it in another | ||
| thread. |
There was a problem hiding this comment.
I'd simplify this as follows:
Fix a regression in 3.13.6 when a call to
:meth:`ssl.SSLSocket.recv <socket.socket.recv>` in a thread would block calling
:meth:`ssl.SSLSocket.send <socket.socket.send>` in another thread.
Indeed, the problem happens even without a dependency between recv and send.
There was a problem hiding this comment.
I don't think we even need to mention send at all. If recv was blocked, it's not possible to access most methods on an SSLSocket, because they'll also try to acquire that same lock.
|
Thanks @ZeroIntensity for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14. |
|
Sorry, @ZeroIntensity, I could not cleanly backport this to |
|
Sorry, @ZeroIntensity, I could not cleanly backport this to |
…nGH-137588) Fixes a deadlock in 3.13.6. (cherry picked from commit 55788a9)
…pythonGH-137588) Fixes a deadlock in 3.13.6. (cherry picked from commit 55788a9) Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
|
GH-137613 is a backport of this pull request to the 3.13 branch. |
Remove locking around
SSLSocketcalls (and more importantly, calls tosendandrecv). I think they were unnecessary, as OpenSSL seems to be fine with those being called concurrently.The test here is likely a bit flaky and I'm not really sure why. It deadlocks on the current(Test now works correctly without flakiness.)main, but also seems to have periodic deadlocks even when e047a35 is not applied. It's either a bug in OpenSSL, an existing bug inssl, or most likely, a bug in theThreadedEchoServerused for the tests. I've added atime.sleep(0)to intentionally yield the GIL, and that seems to somewhat fix the race condition.