Simple Threading, why does this happen? (C# WinForms)

Question

I'm currently exploring threading implementation in C# WinForms and I created this simple app:

I'm just wondering why the memory usage of this app keeps growing after I start, stop, start, and stop again the application. I'm having a thought that my thread instance doesn't really terminate or abort when I press the stop button. Please consider my code below, and any help or suggestions will be greatly appreciated.

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Text;
using System.Windows.Forms;
using System.Threading;

namespace ThreadingTest
{
    public partial class Form1 : Form
    {
        private delegate void TickerDelegate(string s);
        bool stopThread = false;
        TickerDelegate tickerDelegate1;
        Thread thread1;

        public Form1()
        {
            InitializeComponent();
            tickerDelegate1 = new TickerDelegate(SetLeftTicker);
        }

        private void Form1_Load(object sender, EventArgs e)
        {
            thread1 = new Thread(new ThreadStart(disp));
            thread1.Start();
        }

        void disp()
        {
            while (stopThread == false)
            {
                listBox1.Invoke(tickerDelegate1, new object[] { DateTime.Now.ToString() });
                Thread.Sleep(1000);
            }
        }

        private void SetLeftTicker(string s)
        {
            listBox1.Items.Add(s);
        }

        private void btnStop_Click(object sender, EventArgs e)
        {
            stopThread = true;
            if (thread1.IsAlive)
            {
                thread1.Abort();
            }
        }

        private void btnStart_Click(object sender, EventArgs e)
        {
            stopThread = false;
            thread1 = new Thread(new ThreadStart(disp));
            thread1.Start();
        }

        private void btnCheck_Click(object sender, EventArgs e)
        {
            if (thread1.IsAlive)
            {
                MessageBox.Show("Is Alive!");
            }
        }

        private void btnClear_Click(object sender, EventArgs e)
        {
            listBox1.Items.Clear();
        }
    }
}

Answer 1

In btnStop_Click, you set the flag to true, but then immediately check to see if it's still alive. This doesn't give the thread a chance to terminate naturally. Instead, you should perform a wait on the thread using Join (as hydrogen suggests), and if it times out (for whatever reason) then abort the thread.

Answer 2

New threads are pretty expensive in terms of memory. The default stack size is 1MB I believe for each new thread. Others have mentioned that calling Abort on a thread isn't really a good way to end a thread (and if the thread is blocking, it may not even abort the thread.) However, in your case I don't think Abort is the reason you are seeing memory grow.

Basically, the .NET garbage collector probably just has not gotten around to freeing up the memory it has allocated. You could try forcing a collection using GC.Collect() but you shouldn't do this in production code.

Answer 3

OK, there are several recommendations:

Use a Volatile Flag

Make your flag volatile... if you don't, then an update to the flag may never be seen by the thread.

volatile bool stopThread = false;

Set to Background

Set the IsBackground property to true: it forces the thread to be terminated if the application exits, otherwise you may get a "ghost thread" which exists even after the application has closed.

thread1.IsBackground = true;
thread1.Start();

If the thread just started sleeping, then you will abort it even before it has had a chance to read the flag... furthermore you don't want to use Abort because:

...if one thread calls Abort on another thread, the abort interrupts whatever code is running. There is a chance the thread could abort while a finally block is running, in which case the finally block is aborted. There is also a chance that a static constructor could be aborted. In rare cases, this might prevent instances of that class from being created in that application domain.

Use Interrupt instead of Abort

So instead of using abort, I would recommend that you call Interrupt and you handle the exception inside the thread:

private void btnStop_Click(object sender, EventArgs e)
{
    // have another method for re-use
    StopThread();
}

private void StopThread()
{
    stopThread = true;

    // the time out is 100 ms longer than the thread sleep
    thread1.Join(1100);

    // if the thread is still alive, then interrupt it
    if(thread1.IsAlive)
    {
        thread1.Interrupt();
    }
}

Don't "Leak" Threads

You're leaking threads every time you click the Start button... if thread1 is already assigned a thread and you assign another thread to it, then the previous one will continue to exist. You need to stop the previous thread before you start another one.

private void btnStart_Click(object sender, EventArgs e)
{
    // stop the previous thread
    StopThread();

    // create a new thread
    stopThread = false;
    thread1 = new Thread(new ThreadStart(disp));
    thread1.IsBackground = true;// set it to background again
    thread1.Start();
}

Handle the Interrupt

Finally, you need to handle the interrupts in your thread:

void disp()
{
    try
    {
        while (stopThread == false)
        {
            listBox1.Invoke(tickerDelegate1, new object[] { DateTime.Now.ToString() });
            Thread.Sleep(1000);
        }
    }
    catch(ThreadInterruptedException)
    {
        // ignore the exception since the thread should terminate
    }
}

I think that's about it... oh... actually, there is one more thing: Thread Carefully! ;)