Improve disposal logic in AsyncWebsocketMessageResultEnumerator to prevent multiple disposals #476
Conversation
…event multiple disposals
christothes
requested review from
joseharriaga,
trrwilson and
ShivangiReja
as code owners
| @@ -26,8 +27,12 @@ public AsyncWebsocketMessageResultEnumerator(WebSocket webSocket, CancellationTo | |||
|
|
|||
| public ValueTask DisposeAsync() | |||
| { | |||
| ArrayPool<byte>.Shared.Return(_receiveBuffer); | |||
| _webSocket?.Dispose(); | |||
| if (!_disposed) | |||
There was a problem hiding this comment.
this is not thread safe.
There was a problem hiding this comment.
Yes, but do we expect a given instance of the enumerator to be run concurrently?
There was a problem hiding this comment.
probably not in real scenarios, but double return to a pool is such a severe bug. I think we should not be using the pool unless we can guarantee 100% correctness, even in contrived or bug cases or when the user has a bug in their code. Double returns to the pool mess up the whole app domain and can lead issues like data loss.
There was a problem hiding this comment.
Wouldn't hurt to use an Interlocked, e.g.
if (Interlocked.Exchange(ref _receiveBuffer, null) is byte[] toReturn)
{
ArrayPool<byte>.Shared.Return(toReturn);
}There was a problem hiding this comment.
@stephentoub, do we need to make the filed volatile so that there is no use after free?
There was a problem hiding this comment.
Making it volatile won't really help if there's (erroneous) concurrent use of other members of the enumerator, as regardless of whether it's volatile or not they could have already grabbed a snapshot of the field's value. And non-concurrent use doesn't need a fence. Interlocked.Exchange itself is also a full fence.
src/Custom/RealtimeConversation/Internal/AsyncWebsocketMessageEnumerator.cs
Outdated
Show resolved
Hide resolved
src/Custom/RealtimeConversation/Internal/AsyncWebsocketMessageEnumerator.cs
Outdated
Show resolved
Hide resolved
…Enumerator.cs Co-authored-by: Stephen Toub <[email protected]>
closes #474