-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't return SslStreamBuffer when disposing during handshake #68956
Don't return SslStreamBuffer when disposing during handshake #68956
Conversation
Tagging subscribers to this area: @dotnet/ncl, @vcsjones Issue DetailsFixes #66818. This PR makes sure we don't dispose buffers which may be still in use by ongoing handshake, which on debug builds led to an assert firing and crashing entire test sutite (see #65455 for example). Dispose during ongoing read operations was handled correctly before. @wfurt you mentioned we should have some tests which dispose the
```cs
SslProtocols clientProtocol;
SslProtocols serverProtocol;
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
We will not return buffer to the shared pool in that case but it seems ok since that should be rare. (it was not issue prior #64747 as we did not rent buffer for handshake)
cc @stephentoub for any additional concerns.
f080bd2
to
3e24569
Compare
src/libraries/System.Net.Security/src/System/Net/Security/SslStream.IO.cs
Outdated
Show resolved
Hide resolved
…tream.IO.cs Co-authored-by: Stephen Toub <stoub@microsoft.com>
Test failures are unrelated |
Fixes #66818.
This PR makes sure we don't dispose buffers which may be still in use by ongoing handshake, which on debug builds led to an assert firing and crashing entire test sutite (see #65455 for example).
Dispose during ongoing read operations was handled correctly before.
@wfurt you mentioned we should have some tests which dispose the
SslStream
mid-operation on purpose. Would something like below suffice? It is essentially what we did before #66682.</details/