C# WhenAny Cancel call not working causing thread accumulation

Question

It's most likely a really stupid mistake but I cannot figure this out and I've spent almost 2 days on this. I have an app that with a button click launches 5 tasks running in parallel but each with its own delay. This part works fine. However, if you click the same button again, it doesn't cancel previously launched tasks and creates another instance. So basically creates more and more tasks that run in parallel. I obviously have code trying to cancel tasks if the button is clicked more than once but for some reason it doesn't work. Could someone point me to my issue? Or is this code is beyond repair and needs total revamp? Thank you! If you execute this WPF app and click on the button, you will see that ping frequency just keeps on increasing.

using System.Collections.Generic;
using System.ComponentModel;
using System.Runtime.CompilerServices;
using System.Threading;
using System.Threading.Tasks;
using System.Windows;

namespace TestMultiThreadWithDiffSleeps
{
    public partial class MainWindow : Window, INotifyPropertyChanged
    {
        #region Binding
        private string m_output0;
        public string Output0
        {
            get { return m_output0; }
            set { m_output0 = value; OnPropertyChanged(); }
        }

        private string m_output1;
        public string Output1
        {
            get { return m_output1; }
            set { m_output1 = value; OnPropertyChanged(); }
        }

        private string m_output2;
        public string Output2
        {
            get { return m_output2; }
            set { m_output2 = value; OnPropertyChanged(); }
        }

        private string m_output3;
        public string Output3
        {
            get { return m_output3; }
            set { m_output3 = value; OnPropertyChanged(); }
        }

        private string m_output4;
        public string Output4
        {
            get { return m_output4; }
            set { m_output4 = value; OnPropertyChanged(); }
        }

        public event PropertyChangedEventHandler PropertyChanged;
        protected virtual void OnPropertyChanged([CallerMemberName] string propertyName = null)
        {
            if (PropertyChanged != null)
                PropertyChanged.Invoke(this, new PropertyChangedEventArgs(propertyName));
        }
        #endregion

        private static SemaphoreSlim ThreadSemaphore;
        private CancellationTokenSource CancellationTokenSrc;

        public MainWindow()
        {
            InitializeComponent();
            DataContext = this;
            ThreadSemaphore = new SemaphoreSlim(1, 1);
        }

        private async void ButtonStart_Click(object sender, RoutedEventArgs e)
        {
            await StartToMonitor();
        }

        private async Task<bool> StartToMonitor()
        {
            M_Stop(); // Stop everything in case this is a restart

            CancellationTokenSrc = new CancellationTokenSource();
            bool taskResult = await M_Start();
            CancellationTokenSrc = null;

            return taskResult;
        }

        public void M_Stop()
        {
            Output0 = string.Empty;
            Output1 = string.Empty;
            Output2 = string.Empty;
            Output3 = string.Empty;
            Output4 = string.Empty;

            if (CancellationTokenSrc != null)
                CancellationTokenSrc.Cancel();
        }

        private async Task<bool> M_Start()
        {
            List<Task<bool>> tasks = new List<Task<bool>>();

            // Build task list
            for (int i = 0; i < 5; i++)
            {
                int iIdx = i; // Need to do this when multi-threading
                int sleepTime = (i + 1) * 1000;

                tasks.Add(Task.Run(async () =>
                {
                    while (!CancellationTokenSrc.Token.IsCancellationRequested)
                    {
                        if (!Ping(iIdx))
                            return false; // Ping in this example always returns 'true' but you can imagine in real life there'd be 'false' returns that shall cancel all threads

                        await Task.Delay(sleepTime); // Delay for different length of time for each thread
                    }
                    return true;
                }, CancellationTokenSrc.Token));
            }

            Task<bool> firstFinishedTask = await Task.WhenAny(tasks);
            bool result = firstFinishedTask.Result;
            CancellationTokenSrc.Cancel(); // Cancel all other threads as soon as one returns

            return result;
        }

        private bool Ping(int index)
        {
            ThreadSemaphore.Wait(); // Not needed for this app...  here only because it's in my other app I'm troubleshooting

            switch (index)
            {
                case 0: Output0 += "*"; break;
                case 1: Output1 += "*"; break;
                case 2: Output2 += "*"; break;
                case 3: Output3 += "*"; break;
                case 4: Output4 += "*"; break;
            }

            ThreadSemaphore.Release();

            return true;
        }
    }
}


<Window x:Class="TestMultiThreadWithDiffSleeps.MainWindow"
        xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
        xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
        xmlns:d="http://schemas.microsoft.com/expression/blend/2008"
        xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
        mc:Ignorable="d"
        Title="MainWindow" SizeToContent="WidthAndHeight">
    <StackPanel>
        <Button Content="Start" HorizontalAlignment="Left" VerticalAlignment="Top" Width="200" Margin="0,30,0,0" Click="ButtonStart_Click"/>
        <TextBox Text="{Binding Output0}"/>
        <TextBox Text="{Binding Output1}"/>
        <TextBox Text="{Binding Output2}"/>
        <TextBox Text="{Binding Output3}"/>
        <TextBox Text="{Binding Output4}"/>
    </StackPanel>
</Window>

Answer 1

There are a couple of issues in this code.

The source of the problem you're describing is this:

tasks.Add(Task.Run(async () =>
{
    while (!CancellationTokenSrc.Token.IsCancellationRequested)

The tasks always check the token of the current CancellationTokenSrc. So unless the cancellation happens in the exact moment between Task.Delay calls where the tasks check IsCancellationRequested, they'll just see the new, uncancelled CTS you create after a restart.

You should pass the current CancellationToken as a method argument or store it in a local variable inside M_Start, instead of checking the shared CancellationTokenSrc field to avoid this problem.

Slight improvement: Task.Delay also has an overload that accepts a CancellationToken. It will throw a TaskCanceledException.

Additionally, you're updating the OutputX properties from parallel threads (Task.Run), which then raise the PropertyChanged event that's supposed to update the UI. This kind of cross-thread UI interaction isn't safe, you will need to involve the Dispatcher to make sure the event is raised on the UI thread.

Lastly, the ownership of CancellationTokenSrc is fairly complex and seems ripe for race conditions as multiple concurrent methods use and set it. In case of a restart M_Start could easily cause a NullReferenceException when it tries to cancel CancellationTokenSrc which has already been set to null in StartToMonitor.

Answer 2

I think you need to make Task.Delay cancellable too:

await Task.Delay(sleepTime);

Change it to:

await Task.Delay(sleepTime, CancellationTokenSrc.Token);