delay disposal of array buffres if operation is in progress #49657
Conversation
|
Tagging subscribers to this area: @dotnet/ncl, @vcsjones Issue DetailsThis adds extra guard to prevent issues when _inputBuffer and _pendingOutput are disposed during pending operation. I did several hundred runs over night with original fragment of #49573 and I did not see any problems. fixes #49600
|
src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs
Show resolved
Hide resolved
|
|
||
| if (Interlocked.Exchange(ref _pendingOutput, 1) == 2) | ||
| { | ||
| const int writErr = -20; |
There was a problem hiding this comment.
Where does this -20 and the below -19 numbers come from? Are those arbitrary error values, or do they map to something else?
There was a problem hiding this comment.
they are osStatus enums
https://www.osstatus.com/search/results?platform=all&framework=all&search=-19
same as noErr below.
There was a problem hiding this comment.
Could you rename then to const int OSStatus_writErr or something like that?
Or maybe introduce an OSStatus enum somewhere under interop?
There was a problem hiding this comment.
It seems like this does not show up in other places as we map the actual values in PAL via
Looking at this again, I can probably use errSSLClosedAbort = -9806 for both and move it up to top (with noErr) and add comment to it. And use the OSStatus_ prefix. Since this is disposed, I don't think the actually value matters that much.
src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs
Outdated
Show resolved
Hide resolved
| if (Interlocked.Exchange(ref _pendingOutput, 2) == 0) | ||
| { | ||
| _outputBuffer.Dispose(); | ||
| } |
There was a problem hiding this comment.
Would it make sense to wrap ArrayBuffer in a ReservableArrayBuffer, or something like that? Then it would have methods like Rent/Return/Dispose, which would internally do the right synchronization on a ref count or such field.
| _outputBuffer.Dispose(); | ||
| } | ||
|
|
||
| sslContext.Dispose(); |
There was a problem hiding this comment.
I realize this PR is focused on on the buffers, but what does it mean to dispose of the sslContext while that's still in use as well? Is all inappropriate use there handled by this being a SafeHandle, such that actual release of the underlying handle is delayed until all interop with the handle completes (typical SafeHandle stuff)?
There was a problem hiding this comment.
I can take a look. There was previous discussion about using SafeHandles in SslStream. Perhaps that can go together. The buffer part is regression caused by #32338.
| ulong length = (ulong)*dataLength; | ||
| Debug.Assert(length <= int.MaxValue); | ||
|
|
||
| if (Interlocked.Exchange(ref _pendingOutput, NativeIOPending) == Disposed) |
There was a problem hiding this comment.
Should this use CompareExchange?
There was a problem hiding this comment.
We really want to prevent Dispose from running and we want to detect in IO functions what dispose was called to run deferred cleanup. We don't really need to distinguish what is running.
Now looking at this again. It is not safe to manipulate the ArrayBuffer concurrently. We would need lock here to prevent corruption. But the code works because the general flow is like:
- we append received frame to queue.
- we call native code and that consumers the bytes
- it can write more data to the other queue
- we check the queue when finish and we send data to underlying stream
If we go with ReservableArrayBuffer this would turn into counter as you said.
I like it better than the custom state machine so I'll make change as you recommended.
| return noErr; | ||
| } | ||
|
|
||
| if (Interlocked.Exchange(ref _pendingInput, NativeIOPending) == Disposed) |
There was a problem hiding this comment.
Should this use CompareExchange?
|
|
||
| if (_inputBuffer.ActiveLength == 0) | ||
| { | ||
| if (Interlocked.Exchange(ref _pendingInput, NoIOPending) == Disposed) |
There was a problem hiding this comment.
Should this use CompareExchange?
I'll stop commenting on these. Basically, all the ones that are comparing against Disposed seem... suspicious.
|
I had off-line chat with @stephentoub and we have two basis approaches how to go forward:
Do you have preference one way to other @geoffkizer ? |
I vote this. Pay for play in the right place. Don't make everyone pay for the ref counting when it isn't needed. |
+1 That said: In my experience, you usually don't need a full blown ref count here. There are usually only a couple things you are tracking; e.g. (a) whether the stream is disposed, and (b) whether a read is in progress. And you are typically tracking at least some of these (e.g. disposed) anyway, so doing additional addref/release is not really necessary for those cases. In other words, can't we get away with just checking the specific state like disposed and _nestedRead, and ensure we don't release the buffer during a read? |
|
closing this in favor of #49945 |
ghost
locked as resolved and limited conversation to collaborators
5 participants
This adds extra guard to prevent issues when _inputBuffer and _pendingOutput are disposed during pending operation.
SslStream has some logic to guard agains read as _internalBuffer is used but there is nothing about writes so that is particularly problematic. Since _inputBuffer and _pendingOutput are private to the context I decided do it symmetrically for both.
The To/From Connection methods are invoked from Apple's native code. Read & Write are called from managed SslStreamPAL.
I did several hundred runs over night with original fragment of #49573 and I did not see any problems.
cc: @davidfowl
fixes #49600
fixes #43686
contributes to #49573