davidfowl / AspNetCoreDiagnosticScenarios

This repository has examples of broken patterns in ASP.NET Core applications
7.92k stars 750 forks source link

How about long running work that is done using an async method #76

Open alizahid4004 opened 3 years ago

alizahid4004 commented 3 years ago

For something like the following:


public class QueueProcessor
{
    private readonly BlockingCollection _messageQueue = new BlockingCollection();

    public void StartProcessing()
    {
        //Start background processing here. What's the best approach?
    }

    public void Enqueue(Message message)
    {
        _messageQueue.Add(message);
    }

    private async Task ProcessQueue() //notice the async Task here
    {
        foreach (var item in _messageQueue.GetConsumingEnumerable())
        {
             await ProcessItem(item).ConfigureAwait(false);
        }
    }

    private async Task ProcessItem(Message message) { }
}

What is the best approach of executing the long running work in the background? I have looked everywhere and I've found a lot of contradicting advice on this.

newbe36524 commented 3 years ago

I think Channel could be your best choice in most cases. see more: https://devblogs.microsoft.com/dotnet/an-introduction-to-system-threading-channels/

kapsiR commented 3 years ago

What's the use case of your scenario? Why does it even have to be async?
The queue can process the items synchronous on a dedicated thread.
As stated here, it's better to spawn a new thread on your own to prevent blocking a thread from the thread pool.

alizahid4004 commented 3 years ago

@kapsiR Actually I was going through the code of Websocket.Client library and I came across the sending class https://github.com/Marfusios/websocket-client/blob/master/src/Websocket.Client/WebsocketClient.Sending.cs

It uses Channels as @newbe36524 mentions, but it also consumes the items added to the channel on the background task (see line 180).

There's an older article from 2015 by Bar Arnon which calls running async code in long running task like this a bad practice. The article is 6 years old so I don't know if it's applicable today. http://blog.i3arnon.com/2015/07/02/task-run-long-running/

So my original question still stands. What's the best way of consuming work on a background thread?

newbe36524 commented 3 years ago

Please allow me to say simple and direct instructions. First of all, you have some misconceptions.

the following thing is right:

  1. _messageQueue.GetConsumingEnumerable() will block current thread
  2. Task.Run with LongRuning option will create a new thread outside the thread pool

more details:

  1. what http://blog.i3arnon.com/2015/07/02/task-run-long-running/ says is that the LongRunning option is meaningless for code blocks that contain async/await. It is still right for now, because LongRunning will start a separate thread outside the thread pool to execute the code. But if the code has async/await, then after encountering an await, the code reverts back to using the thread in the thread pool for execution. The threads that were intentionally started using LongRunning are then lost. Therefore, this option should only be recommended when there is no async/await in your code. This is not to say that the LongRunning option is not recommended at all times.

  2. If you simply support the above code. Then combine it with the code you gave about WebSocket. Just invoke Task.Run with LongRunning to start a separate thread. Then send the data from the blockCollection to the Channel, and you don't need the async/await keyword in this code. The reason for this is that you should not use Task.Run at this point because the blockCollection's consumption wait is blocking and you will cause one or more threads in the thread pool to block. And then invoke Task.Run w/o LongRunning to receive and consume messages form the Channel. At this point, the consumption code is running in the thread pool, which is efficient enough.

  3. Run with async/await on the consumer side is easy and simple if you can use Channel to pass data in your code from the beginning without BlockCollection.

cameronsjo commented 3 years ago

I asked @davidfowl this on twitter: https://twitter.com/cameroncsjo/status/1442189506145357826?s=20

He requested an issue to be created, but it looks like there already is one.