davidfowl / AspNetCoreDiagnosticScenarios

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

Possible clarification for long-running work advice #85

Open brantburnett opened 2 years ago

brantburnett commented 2 years ago

I'm not sure if it may add more confusion than it's worth, but should an exception for a long-running asynchronous process be included for clarity? By long-running async process I mean a process that runs using async/await so it doesn't block any thread pool threads but which executes for an extended period of time. To my understanding, this pattern is acceptable, though it comes with other complexities.

I've definitely encountered cases where the advice on long-running work was interpreted to also include this pattern. I've seen this pattern used with Task.Factory.StartNew and TaskCreationOptions.LongRunning in the false assumption it puts all the work on a separate thread. In reality, it makes a thread just for startup, and then the first await puts the work back on the thread pool.

Example:

public class WorkerClass : IDisposable
{
    private static readonly TimeSpan WorkInterval = TimeSpan.FromMinutes(5);

    private readonly CancellationTokenSource _cts = new();

    public void Start()
    {
        bool restoreFlow = false;
        try
        {
            if (!ExecutionContext.IsFlowSuppressed)
            {
                ExecutionContext.SuppressFlow();
                restoreFlow = true;
            }

            _ = DoWorkAsync();
        }
        finally
        {
            if (restoreFlow)
            {
                ExecutionContext.RestoreFlow();
            }
        }
    }

    public async Task DoWorkAsync()
    {
        while (!_cts.IsCancellationRequested)
        {
            await Task.Delay(WorkInterval, _cts.Token);

            // Do async work here, assumption is the work item is not CPU intensive
        }
    }

    public void Dispose()
    {
        _cts.Cancel();
        _cts.Dispose();
    }
}
RyanThomas73 commented 2 years ago

An example for these cases would be very helpful. Using Brant Burnett's example above:

Should the task from DoWorkAsync need to be held onto? Should the task from DoWorkAsync be disposed? Is it better to assign the DoWorkAsync() directly to a task field or should it be wrapped in Task.Run call which is also given the cancellation token?