Form method stops Task with cancellation token and waits for task to complete. Stop Method never yields and Task does not stop

Question

I have a Service Class that Starts and Stops a task that is intended to run in the background behind a forms app.

When stopping the monitor: Once the Stop() method is called, no breakpoints are hit on the Monitor Task and IsStarted never becomes false' and is stuck in an infinite loop.

What is wrong with this? Why does the Monitor Task seeming stop running? How do I fix this?

        private volatile bool IsStarted = false;

        public void Start(PartList partList, UnioServiceSettings settings, Action<string, string, string> createSummaryFile)
        {
            _Settings = settings;
            _PartList = partList;
            _cts = new CancellationTokenSource();
            Task.Run(async() => await Monitor(_cts.Token));  //no difference:.ConfigureAwait(false);
            _ProgressMessage?.Report("Monitor Started...");
            IsStarted = true;
        }
            
        public async Task<bool> Stop()
        {
            if (IsStarted)
            {
                _ProgressMessage?.Report("Stopping Monitor...");
                _cts.Cancel();
                while (IsStarted) //this loop runs forever
                {
                    await Task.Delay(1000);  //.ConfigureAwait(false);? no difference
                }
                return true;
            }
            else
            {
                _ProgressMessage?.Report("Cannot stop Monitor. It has not been started.");
                return false;
            }
        }

        private async Task Monitor(CancellationToken token)
        {
            while (!token.IsCancellationRequested)
            {
                 //do stuff
                 await Task.Delay(_Settings.DelayMs,token).ConfigureAwait(false);
            }
            IsStarted = false;
            return;
        }

Answer 1

There is a race condition here:

Task.Run(async() => await Monitor(_cts.Token));
_ProgressMessage?.Report("Monitor Started...");
IsStarted = true;

It is possible that the Task created by the Task.Run method will complete on another thread before the current thread calls IsStarted = true;. So eventually the IsStarted will settle at the value true without a Task running. To solve this problem you could transfer the responsibility of mutating the IsStarted field exclusively to the Monitor method itself:

private async Task Monitor(CancellationToken token)
{
    IsStarted = true;
    try
    {
        _ProgressMessage?.Report("Monitor Started...");
        while (true)
        {
            token.ThrowIfCancellationRequested();
            //do stuff
            await Task.Delay(_Settings.DelayMs,token).ConfigureAwait(false);
        }
    }
    finally
    {
        IsStarted = false;
    }
}

I don't know if this race condition is the cause of your problem though.

---

Update: Here is another approach at implementing your service, where the bool IsStarted field has been replaced with a Task _monitorTask field. This implementation assumes that a caller will not invoke the methods Start and Stop concurrently. It actually enforces the no-concurrency policy, with the help of a SemaphoreSlim(1, 1).

private readonly SemaphoreSlim _semaphore = new(1, 1);
private CancellationTokenSource _cts;
private Task _monitorTask;
private IProgress<string> _progressMessage;

public void Start()
{
    if (!_semaphore.Wait(0)) throw new InvalidOperationException("Concurrency!");
    try
    {
        if (_monitorTask != null && !_monitorTask.IsCompleted)
            throw new InvalidOperationException("The Monitor is already running!");
        _progressMessage?.Report("Starting Monitor...");
        _cts = new();
        CancellationToken token = _cts.Token;
        _monitorTask = Task.Run(() => MonitorAsync(token), token);
    }
    finally { _semaphore.Release(); }
}

public async Task<bool> Stop()
{
    if (!_semaphore.Wait(0)) throw new InvalidOperationException("Concurrency!");
    try
    {
        if (_monitorTask == null)
        {
            _progressMessage?.Report("Monitor has not been started.");
            return false;
        }
        else if (_monitorTask.IsCompleted)
        {
            _progressMessage?.Report("Monitor is already stopped.");
            return false;
        }
        else
        {
            _progressMessage?.Report("Stopping Monitor...");
            _cts?.Cancel();
            try { await _monitorTask; } catch { } // Ignore exceptions here
            _cts?.Dispose(); _cts = null;
            return true;
        }
    }
    finally { _semaphore.Release(); }
}

private async Task MonitorAsync(CancellationToken token)
{
    try
    {
        _progressMessage?.Report("Monitor started.");
        while (true)
        {
            // Do stuff
            await Task.Delay(1000, token);
        }
    }
    catch (OperationCanceledException) when (token.IsCancellationRequested)
    {
        _progressMessage?.Report("Monitor stopped.");
    }
    catch (Exception ex)
    {
        _progressMessage?.Report($"Monitor failed: {ex.Message}");
    }
}

Notice that the _cts.Token is stored in a local variable inside the Start method, in order to prevent a possible race condition. The Task.Run action is invoked on another thread, most probably after the completion of the Start method. We don't want this action to interact with the _cts field, which must be handled from concurrency-protected regions only.