Skip to content

Improve disposal logic in AsyncWebsocketMessageResultEnumerator to prevent multiple disposals #476

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.

Sign up for GitHub

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

Merged
KrzysztofCwalina merged 3 commits into from
Jun 24, 2025
Merged

Improve disposal logic in AsyncWebsocketMessageResultEnumerator to prevent multiple disposals #476

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e061c35
Improve disposal logic in AsyncWebsocketMessageResultEnumerator to pr…
christothes Jun 23, 2025
1cf836b
fb
christothes Jun 24, 2025
6699f39
Update src/Custom/RealtimeConversation/Internal/AsyncWebsocketMessage…
christothes Jun 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Improve disposal logic in AsyncWebsocketMessageResultEnumerator to pr… 
…event multiple disposals
  • Loading branch information
@christothes
christothes committed Jun 23, 2025
commit e061c35916f40c56fbffea0fdae31a0c53159f2b
9 changes: 7 additions & 2 deletions src/Custom/RealtimeConversation/Internal/AsyncWebsocketMessageEnumerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ internal partial class AsyncWebsocketMessageResultEnumerator : IAsyncEnumerator<
private readonly CancellationToken _cancellationToken;
private readonly WebSocket _webSocket;
private readonly byte[] _receiveBuffer;
private bool _disposed = false;

public AsyncWebsocketMessageResultEnumerator(WebSocket webSocket, CancellationToken cancellationToken)
{
Expand All @@ -26,8 +27,12 @@ public AsyncWebsocketMessageResultEnumerator(WebSocket webSocket, CancellationTo

public ValueTask DisposeAsync()
{
ArrayPool<byte>.Shared.Return(_receiveBuffer);
_webSocket?.Dispose();
if (!_disposed)
Copy link
Collaborator

@KrzysztofCwalina KrzysztofCwalina Jun 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not thread safe.

Copy link
Collaborator Author

@christothes christothes Jun 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but do we expect a given instance of the enumerator to be run concurrently?

Copy link
Collaborator

@KrzysztofCwalina KrzysztofCwalina Jun 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@stephentoub stephentoub Jun 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't hurt to use an Interlocked, e.g.

if (Interlocked.Exchange(ref _receiveBuffer, null) is byte[] toReturn)
{
    ArrayPool<byte>.Shared.Return(toReturn);
}

Copy link
Collaborator

@KrzysztofCwalina KrzysztofCwalina Jun 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stephentoub, do we need to make the filed volatile so that there is no use after free?

Copy link
Contributor

@stephentoub stephentoub Jun 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@KrzysztofCwalina KrzysztofCwalina Jun 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true

{
ArrayPool<byte>.Shared.Return(_receiveBuffer);
_webSocket?.Dispose();
_disposed = true;
}
return new ValueTask(Task.CompletedTask);
}

Expand Down