Using SemaphoreSlim, update UI while waiting for all threads to finish

Question

I've recently started to study C# and get stuck on running multiple threads while updating the UI. From what I've learned so far, SemaphoreSlim seems to the right way to run multiple threads while still controlling the maximum number of concurrent threads.

The scenario: I want to send a GET request to a website (e.g. http://www.somesite.com/keyword) and use the returned string for a large number of keywords. While running all threads, after each thread I want to update UI like counting good results and bad ones. This is my code so far:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Net;
using System.Threading;
using System.Windows.Forms;

namespace WindowsFormsApp1
{
    public partial class Form1 : Form
    {
        static SemaphoreSlim _pool;
        static readonly int maxThreads = 4;// able to go up to 100-200
        static List<string> keysList = new List<string>();
        static List<string> liveKeys = new List<string>();
        static List<string> deadKeys = new List<string>();
        static Queue<string> keysQueue = new Queue<string>();
        static int liveCount = 0;
        static int deadCount = 0;

        public Form1()
        {
            InitializeComponent();
            getAllKeyWordsToList();// get keywords from file
        }

        private void startButton_Click(object sender, EventArgs e)
        {
            _pool = new SemaphoreSlim(maxThreads);
            foreach (string keyWord in keysList)
            {
                keysQueue.Enqueue(keyWord);
            }


            foreach (string key in keysList)
            {
                new Thread(Worker).Start(key);
                keysQueue.Dequeue();
            }

            // this part is skipped when startButton is pressed
            // tried with looks but I'm too stupid or too new with C#
            while(keysQueue.Count() > 0)
            {
                // update UI
                this.statusLabel.Text = "Only " + keysQueue.Count().ToString() + " keywords left";
                this.liveLabel.Text = liveLabel.ToString();
                this.deadLabel.Text = deadLabel.ToString();
            }

            // this get's updated; only this... 
            // while threads are still running
            this.statusLabel.Text = "Finished!";
        }

        private void Worker(object obj)
        {
            var uri = "http://www.somesite.com/";
            var key = obj.ToString();

            using(var wc = new WebClient())
            {
                string result = wc.DownloadString(uri + key);

                _pool.Wait();
                if (result.Contains("live"))
                {
                    // do some more work with the result
                    liveCount++;
                }
                else
                {
                    // do some work with the result
                    deadCount++;
                }
                _pool.Release();
            }
        }
    }
}

Answer 1

You do not need, nor should use, SemaphoreSlim here.

The reason that your code skips the while loop is that you've emptied the queue by the time you get there. This loop:

foreach (string key in keysList)
{
    new Thread(Worker).Start(key);
    keysQueue.Dequeue();
}

…starts a new thread for each element in the keysList collection, and removes an item (without even using the value!) from the queue. None of that blocks in any way; the thread starts independently of the loop, and doesn't affect the loop's progress. So the loop completes, most likely before even a single thread has even started running, but in any case almost certainly before any has finished, and the code moves on to execute the while loop, with the keysQueue collection already emptied by the previous foreach loop, above.

Since the queue's Count is already 0 at that point, the while loop is never entered. The condition is already false on the first attempt to execute it.

Beyond that, your code has a number of other aspects that are sub-par. The biggest ones being, besides that you are trying to use a semaphore where none is needed, that the code allocates a whole thread to each active operation, and that it blocks the UI (or, at least, would block the UI if the code had done what you wanted) while the whole processing goes on.

The WebClient class has a DownloadStringAsync() method, which can be used to perform each download operation asynchronously in an "awaitable" manner. "Awaitable" means that the method can return before the operation has completed, allowing the current thread to keep working (i.e. in this case so that it can handle UI updates and even user input if desired) while the operation progresses.

By using this asynchronous version of the DownloadString() method, and by keeping track of the tasks as they are started, it's easy to write a straight-forward loop that does all the processing you want, throttling the operations to whatever maximum concurrent operations you want, all without blocking the UI thread.

Here is an example of what I mean:

public partial class Form1 : Form
{
    private const int _kmaxTasks = 4;
    private readonly List<string> _keysList =
        new List<string> { "a", "b", "c", "d", "e", "f", "g", "h", "i", "j" };
    private readonly List<string> _liveKeys = new List<string>();
    private readonly List<string> _deadKeys = new List<string>();

    public Form1()
    {
        InitializeComponent();
    }

    private async void button1_Click(object sender, EventArgs e)
    {
        // Initialize a new queue, copying all elements from _keysList to the queue
        Queue<string> keysQueue = new Queue<string>(_keysList);

        // List of tasks, to keep track of active tasks
        //
        // NOTE: value tuple syntax requires that you use the NuGet Package Manager
        // to get Microsoft's "ValueTuple" package installed for your project.
        List<Task<(bool, string)>> tasks = new List<Task<(bool, string)>>();

        button1.Enabled = false;
        _liveKeys.Clear();
        _deadKeys.Clear();
        _UpdateStatus(keysQueue.Count, _liveKeys.Count, _deadKeys.Count);

        // Keep working until we're out of keys *and* out of tasks
        while (keysQueue.Count > 0 || tasks.Count > 0)
        {
            // If we've got the max number of tasks running already, wait for one to
            // complete. Even if we don't have the max number of tasks running, if we're
            // out of keys also wait for one to complete.
            if (tasks.Count >= _kmaxTasks || keysQueue.Count == 0)
            {
                Task<(bool Live, string Key)> completedTask = await Task.WhenAny(tasks);

                tasks.Remove(completedTask);

                if (completedTask.Result.Live)
                {
                    _liveKeys.Add(completedTask.Result.Key);
                }
                else
                {
                    _deadKeys.Add(completedTask.Result.Key);
                }

                _UpdateStatus(
                    keysQueue.Count + tasks.Count, _liveKeys.Count, _deadKeys.Count);
            }

            if (keysQueue.Count > 0)
            {
                tasks.Add(Worker(keysQueue.Dequeue()));
            }
        }

        statusLabel.Text = "Finished!";
        button1.Enabled = true;
    }

    private void _UpdateStatus(int count, int liveCount, int deadCount)
    {
        statusLabel.Text = $"Only {count} keywords left";
        liveLabel.Text = liveCount.ToString();
        deadLabel.Text = deadCount.ToString();
    }

    private async Task<(bool, string)> Worker(string key)
    {
        string uri = "http://www.somesite.com/";

        using (MockWebClient wc = new MockWebClient())
        {
            string result = await wc.DownloadStringAsync(uri + key);

            return (result.Contains("live"), key);
        }
    }
}

Note that in addition to converting the code to use asynchronous operations, and of course to work correctly, all of which in and of itself simplified the code a lot, I've also removed unnecessary variables and moved the queue object into the Click event handler, referenced only by a local variable.

I've also refactored the Worker a little bit, as IMHO it makes more sense for the Worker() method to just do the download and check for "live", and then let the caller handle the bookkeeping based on the result. In doing so, I've also used the new C# value-tuple feature, which allows me to return a pair of values from the method using a simplified built-in syntax to represent them.

Finally, since your question doesn't include any specific web server or key values that could actually be used to test the code (for future reference, please be mindful of the need to provide a good Minimal, Complete, and Verifiable code example that correctly illustrates your question), I used some dummy test data, and wrote a simple MockWebClient that has the same DownloadStringAsync() method, but with an implementation that just pretends to do some work and return a result, using a random number generator to determine the duration of the operation and the result itself:

class MockWebClient : IDisposable
{
    private static readonly TimeSpan _kminDelay = TimeSpan.FromSeconds(1);
    private static readonly TimeSpan _kmaxDelay = TimeSpan.FromSeconds(5);

    private static readonly Random _random = new Random();
    private static readonly object _lock = new object();

    private static TimeSpan _NextRandomDelay(TimeSpan min, TimeSpan max)
    {
        lock (_lock)
        {
            return TimeSpan.FromSeconds(
                (max.TotalSeconds - min.TotalSeconds) * _random.NextDouble());
        }
    }

    private static bool _NextRandomBool()
    {
        lock (_lock)
        {
            return _random.Next(2) == 1;
        }
    }

    public async Task<string> DownloadStringAsync(string uri)
    {
        await Task.Delay(_NextRandomDelay(_kminDelay, _kmaxDelay));
        return _NextRandomBool() ? "live" : "dead";
    }

    public void Dispose()
    {
        // do nothing...it's a mock!
    }
}

Obviously that class isn't needed for your real-world program. It's there just so you (and I) can run the code and see it working without having to deal with the real web server.

Finally, here's the *.Designer.cs code, for completeness:

partial class Form1
{
    /// <summary>
    /// Required designer variable.
    /// </summary>
    private System.ComponentModel.IContainer components = null;

    /// <summary>
    /// Clean up any resources being used.
    /// </summary>
    /// <param name="disposing">true if managed resources should be disposed; otherwise, false.</param>
    protected override void Dispose(bool disposing)
    {
        if (disposing && (components != null))
        {
            components.Dispose();
        }
        base.Dispose(disposing);
    }

    #region Windows Form Designer generated code

    /// <summary>
    /// Required method for Designer support - do not modify
    /// the contents of this method with the code editor.
    /// </summary>
    private void InitializeComponent()
    {
        this.button1 = new System.Windows.Forms.Button();
        this.label1 = new System.Windows.Forms.Label();
        this.label2 = new System.Windows.Forms.Label();
        this.label3 = new System.Windows.Forms.Label();
        this.liveLabel = new System.Windows.Forms.Label();
        this.deadLabel = new System.Windows.Forms.Label();
        this.statusLabel = new System.Windows.Forms.Label();
        this.SuspendLayout();
        // 
        // button1
        // 
        this.button1.Location = new System.Drawing.Point(13, 13);
        this.button1.Name = "button1";
        this.button1.Size = new System.Drawing.Size(182, 60);
        this.button1.TabIndex = 0;
        this.button1.Text = "Start";
        this.button1.UseVisualStyleBackColor = true;
        this.button1.Click += new System.EventHandler(this.button1_Click);
        // 
        // label1
        // 
        this.label1.AutoSize = true;
        this.label1.Location = new System.Drawing.Point(40, 110);
        this.label1.Name = "label1";
        this.label1.Size = new System.Drawing.Size(83, 32);
        this.label1.TabIndex = 1;
        this.label1.Text = "Live: ";
        // 
        // label2
        // 
        this.label2.AutoSize = true;
        this.label2.Location = new System.Drawing.Point(25, 142);
        this.label2.Name = "label2";
        this.label2.Size = new System.Drawing.Size(98, 32);
        this.label2.TabIndex = 1;
        this.label2.Text = "Dead: ";
        // 
        // label3
        // 
        this.label3.AutoSize = true;
        this.label3.Location = new System.Drawing.Point(12, 174);
        this.label3.Name = "label3";
        this.label3.Size = new System.Drawing.Size(111, 32);
        this.label3.TabIndex = 1;
        this.label3.Text = "Status: ";
        // 
        // liveLabel
        // 
        this.liveLabel.AutoSize = true;
        this.liveLabel.Location = new System.Drawing.Point(123, 110);
        this.liveLabel.Name = "liveLabel";
        this.liveLabel.Size = new System.Drawing.Size(0, 32);
        this.liveLabel.TabIndex = 1;
        // 
        // deadLabel
        // 
        this.deadLabel.AutoSize = true;
        this.deadLabel.Location = new System.Drawing.Point(123, 142);
        this.deadLabel.Name = "deadLabel";
        this.deadLabel.Size = new System.Drawing.Size(0, 32);
        this.deadLabel.TabIndex = 1;
        // 
        // statusLabel
        // 
        this.statusLabel.AutoSize = true;
        this.statusLabel.Location = new System.Drawing.Point(123, 174);
        this.statusLabel.Name = "statusLabel";
        this.statusLabel.Size = new System.Drawing.Size(0, 32);
        this.statusLabel.TabIndex = 1;
        // 
        // Form1
        // 
        this.AutoScaleDimensions = new System.Drawing.SizeF(16F, 31F);
        this.AutoScaleMode = System.Windows.Forms.AutoScaleMode.Font;
        this.ClientSize = new System.Drawing.Size(1159, 780);
        this.Controls.Add(this.label3);
        this.Controls.Add(this.label2);
        this.Controls.Add(this.statusLabel);
        this.Controls.Add(this.deadLabel);
        this.Controls.Add(this.liveLabel);
        this.Controls.Add(this.label1);
        this.Controls.Add(this.button1);
        this.Name = "Form1";
        this.Text = "Form1";
        this.ResumeLayout(false);
        this.PerformLayout();

    }

    #endregion

    private System.Windows.Forms.Button button1;
    private System.Windows.Forms.Label label1;
    private System.Windows.Forms.Label label2;
    private System.Windows.Forms.Label label3;
    private System.Windows.Forms.Label liveLabel;
    private System.Windows.Forms.Label deadLabel;
    private System.Windows.Forms.Label statusLabel;
}