Method called within thread executes in unexpected order (C# .net)

272 views Asked by At

seeking general help in understanding this program flow: In a windows forms application why is it that when I call a method within a new thread that the thread doesn't wait for the method to complete before continuing execution?

Are methods called within a thread executed asynchronously by default? (I would expect the program to block until the method is complete without having to use the Thread.Sleep line below). The comment on the "Thread.Sleep" line below may help to clarify my question further. - Thank you!

    private void button1_Click(object sender, EventArgs e)
    {
        //Call doStuff in new thread:
        System.Threading.Thread myThread;
        myThread = new System.Threading.Thread(new System.Threading.ThreadStart(doStuff));
        myThread.Start();
    }

    private void doStuff()
    {
        //Instantiate external class used for threadSafe interaction with UI objects:
        ThreadSafeCalls tsc = new ThreadSafeCalls();
        int indx_firstColumn = 0;

        //Loop 3 times, add a row, and add value of "lastRowAdded" to column1 cell.
        for (int a = 0; a < 3; a += 1)
        {
            tsc.AddGridRow(dataGridView1); //Call method to add a row to gridview:
            Thread.Sleep(1000); // Why does the execution of the line above go all crazy if I don't pause here? It's executing on the same thread, shouldn't it be synchronous? 
            tsc.AddGridCellData(dataGridView1,indx_firstColumn, tsc.lastRowAdded,tsc.lastRowAdded.ToString()); //Add value of "lastRowAdded" (for visual debugging)
        }

Content of the "ThreadSafeCalls" class:

        public int lastRowAdded = -999;

    public void AddGridRow(DataGridView gv)
    {
        if (gv.InvokeRequired)
        {
            gv.BeginInvoke((MethodInvoker)delegate () 
            {
                lastRowAdded = gv.Rows.Add();
            });
        }
        else
        {
            lastRowAdded = gv.Rows.Add();
        }
    }

    public void AddGridCellData(DataGridView gv, int column, int rowID, string value)
    {
        //Thread safe:
            if (gv.InvokeRequired)
            {
                gv.BeginInvoke((MethodInvoker)delegate () { gv.Rows[rowID].Cells[column].Value = value + " "; });
            }
            else
            {
                gv.Rows[rowID].Cells[column].Value = value;
            }
    }
2

There are 2 answers

8
Koby Duck On BEST ANSWER

Edit: It's not exactly clear what you mean by "go all crazy", but after staring at the code for an unhealthy amount of time I realized there's a race window that manifests itself in the loop when sleeping isn't used. I ran a stripped version of this many times to confirm. Apologies for the long edit, but without details it's difficult to understand the problem.

//Loop 3 times, add a row, and add value of "lastRowAdded" to column1 cell.
for (int a = 0; a < 3; a += 1)
{
    tsc.AddGridRow(dataGridView1); //Call method to add a row to gridview:
    Thread.Sleep(1000); // Why does the execution of the line above go all crazy if I don't pause here? It's executing on the same thread, shouldn't it be synchronous? 
    tsc.AddGridCellData(dataGridView1,indx_firstColumn, tsc.lastRowAdded,tsc.lastRowAdded.ToString()); //Add value of "lastRowAdded" (for visual debugging)
}

Breaking it down:

  1. Schedule the addition of a row and update of lastRowAdded on the UI thread.
  2. Sleep for 1s. Leaving this out causes the race to manifest.
  3. Pass the value and string equivalent of lastRowAdded as myThread remembers it, scheduling the cell to be updated on the UI thread.
  4. Repeat 3 times.

The reason you're experiencing this is due to caching. Essentially when you leave out the sleep myThread sees a stale version of lastRowAdded, which then passes a stale copy to AddGridCellData. This stale value propagates to the UI thread, often resulting in -999 or some other incorrect row index. You should be getting an IndexOutOfRangeException sometimes, but not always. Sleeping happens to give the UI thread enough time to write its cached values back to main memory, where myThread then reads the updated value. It appears to work correctly on some runs and incorrectly on others depending on which cores the OS schedules the threads on.

To fix this you need to remove the sleep and synchronize access to all mutable data inside ThreadSafeCalls. The simplest way to do that is to use a lock.

i.e.

Loop implementation:

for (int a = 0; a < 3; a += 1)
{
    tsc.AddGridRow(dataGridView1);
    tsc.AddGridCellData(dataGridView1, indx_firstColumn);
}

TSC implementation:

class ThreadSafeCalls
{
    private object syncObject = new object();
    private int lastRowAdded = -999;

    public int LastRowAdded
    {
        get {
            lock (syncObject) {
                return lastRowAdded;
            }
        }
        set {
            lock (syncObject) {
                lastRowAdded = value;
            }
        }
    }

    public void AddGridRow(DataGridView gv)
    {
        if (gv.InvokeRequired) {
            gv.BeginInvoke((MethodInvoker)delegate () {
                LastRowAdded = gv.Rows.Add();
            });
        }
        else {
            LastRowAdded = gv.Rows.Add();
        }
    }
    public void AddGridCellData(DataGridView gv, int column)
    {
        if (gv.InvokeRequired) {
            gv.BeginInvoke((MethodInvoker)delegate () {
                var lastRow = LastRowAdded;
                gv.Rows[lastRow].Cells[column].Value = lastRow + " "; 
            });
        } else {
            var lastRow = LastRowAdded;
            gv.Rows[lastRow].Cells[column].Value = lastRow + " ";
        }
    }
}

Original Answer:

Are methods called within a thread executed asynchronously by default?

Yes when referring to execution of the ThreadStart target method with respect to the creating thread; that's the point of threads. You use additional threads to do things like load in the background or additional processing without blocking your main thread.

(I would expect the program to block until the method is complete without having to use the Thread.Sleep line below).

BeginInvoke adds a method to the UI thread scheduler to be invoked later on the UI thread when it's done with whatever it was doing prior(i.e. handling OS events, executing a method scheduled before the one you're adding). This is deferred execution, and returns as soon as it's scheduled, that's why it's not blocking. The reason execution is deferred to the UI thread is that most of the UI framework isn't thread-safe. Multiple threads modifying the UI leads to data races, creating all sorts of problems.

On a side note, you should really avoid creating threads in response to user input. Threads are expensive resources that should be created as soon as possible(ideally at initialization time). In your example you're creating a thread every time your button is clicked, which is very slow.

0
tmaj On

From search results:

Control.BeginInvoke Method (System.Windows.Forms)

Executes a delegate asynchronously on the thread that the control's underlying handle was created on. Executes the specified delegate asynchronously on the thread

Source: Control.BeginInvoke Method on msdn.