refactor: use Channels as queueing mechanism for periodic websocket messages #11092
Conversation
…essages The old implementation used async void as the signature for the event handlers, which might lead to thread pool exhaustion as handlers would fetch the data synchronously despite the async signature on GetDataToSend. Furthermore, on every event, the list of active connections were retrieved inside a `lock` statement, which could also lead to threads being blocked during high traffic scenarios. Queueing a bool is not great design, but this change only aims to alleviate the thread pool exhaustion.
| @@ -137,63 +144,66 @@ protected void SendData(bool force) | |||
|
|
|||
| private async Task HandleMessages() | |||
| { | |||
| await foreach (var force in _channel.Reader.ReadAllAsync()) | |||
| while (await _channel.Reader.WaitToReadAsync().ConfigureAwait(false)) | |||
There was a problem hiding this comment.
Why not keeping ReadAllAsync? looking at the source code it does the same WaitToRead/TryRead
There was a problem hiding this comment.
We don't really need an IAsyncEnumerable. That one is mostly for when you know the Writer will complete at some point. At least according to the docs.
There was a problem hiding this comment.
i see, i think the description is just misleading.
https://github.com/dotnet/runtime/blob/4e86b1c63d9c41c6bfb6f42710be907199ce2671/src/libraries/System.Threading.Channels/src/System/Threading/Channels/ChannelReader.cs#L103
Its essentially the same as the while/if but in one line
There was a problem hiding this comment.
One is a few loops, the other is looping over an async enumerator, so not quite the same. We don't need an IAsyncEnumerable.
The old implementation used async void as the signature for the event handlers, which might lead to thread pool exhaustion as handlers would fetch the data synchronously despite the async signature on GetDataToSend. Furthermore, on every event, the list of active connections were retrieved inside a
lockstatement, which could also lead to threads being blocked during high traffic scenarios.Queueing a bool is not great design, but this change only aims to alleviate the thread pool exhaustion.
Issues
Might fix #7783, but I have not been able to reproduce it.