This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
Fix threading issue when reading and writing from SslStream at the same time #37736
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.
Fixes #36426 - note, this is not scoped to Alpine - issue repros on any Linux with latest (1.1.1) OpenSSL installed.
There were also occasional process crashes I've observed disappearing after fixing this (didn't get a chance to capture stack trace as it happened fairly rarely and I initially thought it was due to bad setup/unrelated)
Details:
SSL_read, SSL_write, SSL_get_error are advertised as they are not designed to work in multithreaded scenarios when working on the same SSL connection: https://www.openssl.org/docs/faq.html#PROG1
for HTTP 1 scenarios this is rather unusual to read and write at the same time but for HTTP 2 this is common.
Note: multiple reads OR writes (but not reads and writes) at the same time can still cause some issues around BIO_read/BIO_write (BIO_* use separate queues for reads and writes) - I have not addressed them because multiple reads at the same time do not seem to be useful in any scenarios (user will essentially get random bytes since you can't reason about the order).
This is marked as draft because of the pending investigation:
cc: @wfurt @stephentoub
Before
After
End to end tests (aspnet/Benchmarks: PlaintextNonPipelined)
Numbers seem to have difference within the error margin which seems like it might not be worth to investigate further and just take a fix (we might want to check also more end to end scenarios)
cc: @sebastienros