Lock contention in SslStream.Encrypt/Decrypt

[davidfowl]

Description

Continuing looking at various profiles that include HTTPS and not just looking at more than just CPU profiles since these benchmarks are IO bound. I'm reporting what I see in the profiles.

Configuration

• .NET Core 7.0.0-preview.5.22251.5+5f7f7e8
• Linux x64
• This is an HTTP/2 scenario

Regression?

Nope

Data

Analysis

I haven't dug deeply into this yet but I'm a little confused why the handshake lock has any contention beyond the handshake (

runtime/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs

Line 146 in 6ca8c9b

lock (_handshakeLock)

and

runtime/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs

Line 788 in 6ca8c9b

lock (_handshakeLock)

).

Profile attached:
application.05-02-19-35-44.zip

Tagging subscribers to this area: @dotnet/ncl, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Description

Continuing looking at various profiles that include HTTPS and not just looking at more than just CPU profiles since these benchmarks are IO bound. I'm reporting what I see in the profiles.

Configuration

• .NET Core 7.0.0-preview.5.22251.5+5f7f7e8
• Linux x64

Regression?

Nope

Data

Analysis

I haven't dug deeply into this yet but I'm a little confused why the handshake lock has any contention beyond the handshake (

runtime/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs

Line 146 in 6ca8c9b

lock (_handshakeLock)

and

runtime/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs

Line 788 in 6ca8c9b

lock (_handshakeLock)

)

Author:	davidfowl
Assignees:	-
Labels:	area-System.Net.Security, tenet-performance, untriaged
Milestone:	-

[wfurt]

It is not safe to encrypt and decrypt at the same time. The lock used to be in OpenSSL PAL but got moved and shared with handshake because of TLS 1.3 windows behavior.

[wfurt]

Here is the original PR dotnet/corefx#37736

[davidfowl]

Hmm those end to end benchmarks are suspicious... AFAIK we don't have PlaintextNonPipelined that runs with https.

[wfurt]

There still may be some opportunity for improvements. For example, this also covers reading from BIO and copying data. AFAIK we only need to prevent SSL state being modified from two threads. On Windows the bug deal is Tls13. It uses the handshake code path after the handshakes is finished and concurrent writes fail with some weird error. So we combined the locks to one (end left it perhaps badly named)

[antonfirsov]

Triage: we should investigate this in 7.0

[wfurt]

https://www.openssl.org/docs/faq.html#PROG1

Is OpenSSL thread-safe?

Yes but with some limitations; for example, an SSL connection cannot be used concurrently by multiple threads. This is true for most OpenSSL objects.
For version 1.1.0 and later, there is nothing further you need do.
For earlier versions than 1.1.0, it is necessary for your application to set up the thread callback functions. To do this, your application must call CRYPTO_set_locking_callback(3) and one of the CRYPTO_THREADID_set… API’s. See the OpenSSL threads manpage for details and “note on multi-threading” in the INSTALL file in the source distribution.

The "handshake" name comes from Windows. It can actually go through the handshake logic from `Decrypt' during Tls13 negotiation and more rarely during renegotiation.

[stephentoub]

We should:

• Rename the lock object; it's confusing :)
• Avoid locking entirely in situations where we don't need it, e.g. do we need it on Windows for TLS versions other than TLS 1.3?
• Reduce the scope of the locking to the bare minimum that actually requires mutual exclusion (and if any of that is because of management of our own managed state, find a way to avoid the need for that)