Please help me make this code thread safe

Question

I've got a bit of a problem with making my data loading and filtering thread safe.

The following code on my control's base class which handles all the data population through a BackgroundWorker. This tends to throw the error on "this.DataWorker.RunWorkerAsync()" saying that the BackgroundWorker is busy.

/// <summary>
/// Handles the population of the form data.
/// </summary>
/// <param name="reload">Whether to pull data back from the WebService.</param>
public void Populate(bool reload)
{
    if (!this.DataWorker.IsBusy)
    {

        // Disable the filter options
        IvdSession.Instance.FilterManager.SetEnabledState(this.GetType(), false);

        // Perform the population
        this.DataWorker.RunWorkerAsync(reload);

    }
    else if (!reload)
    {
        // If the data worker is busy and this is a not reload, then something bad has happened (i.e. the filter has run during a reload.)
        throw new InvalidOperationException("The DataWorker was busy whilst asked to reload.");
    }
}

The code is called in two possible places. Firstly by a timer on the form that the control is on:

private void tmrAutoRefresh_Tick(object sender, EventArgs e)
{
    if (!(this.CurrentBody == null))
    {
        this.CurrentBody.Populate(true);
    }
}

And secondly, any time a user selects a Filter Option from a number of drop down lists:

public void Filter()
{
    if (!m_BlockFilter)
    {
        IvdInstance.Main.CurrentBody.FirstRun = true;
        IvdInstance.Main.CurrentBody.Populate(false);
    }
}

The Timer on the main form runs every 60 seconds and passes true to the Populate method. Passing reload as trues tells the BackgroundWorker that it needs to pull down a fresh set of data from the WebService:

void dataWorker_DoWork(object sender, DoWorkEventArgs e)
{

    try
    {

        if (base.FirstRun)
        {
            base.CleanListView();
        }

        if ((bool)e.Argument)
        {
            byte[] serialized = IvdSession.DataAccess.GetServiceCalls(IvdSession.Instance.Company.Description, IvdSession.Instance.Company.Password, null);
            m_DataCollection = new DalCollection<ServiceCallEntity>(serialized);
        }

        List<ServiceCallEntity> collection = this.ApplyFilter();
        base.HandlePopulation<ServiceCallEntity>(collection, e);

    }
    catch (WebException ex)
    {
        // Ignore - Thrown when user clicks cancel
    }
    catch (System.Web.Services.Protocols.SoapException ex)
    {
        // Log error on server and stay transparent to user
        base.LogError(ex);
    }
    catch (System.Data.SqlClient.SqlException ex)
    {
        // Inform user that the database is unavailable
        base.HandleSystemUnavailable(ex);
    }

}

As far as I'm aware, the error occurs when I manage to click a filter option at exactly the same time the Timer fires the population event. I figure there is something missing from the Populate method, i.e. a lock, but I'm unsure as to how to use it correctly in this instance.

The code is favoured towards the user input. If a user selects a filter option, the auto update should be blocked, if the auto update fires then the filter options are temporarily disabled. If they fire at the same time, the user input should get priority (if possible).

Hope someone can help!

Answer 1

See Thread Synchronization (C# Programming Guide):

public class TestThreading
{
    private System.Object lockThis = new System.Object();

    public void Function()
    {

        lock (lockThis)
        {
            // Access thread-sensitive resources.
        }
    }
}

Edit: You don't want two threads entering Populate, so you could do something as bellow:

public void Populate(bool reload)
{

    lock (lockThis)
    {
        // Disable the filter options
        IvdSession.Instance.FilterManager.SetEnabledState(this.GetType(), false);

        // do actual work.
    }

}

Edit2: You got good thing going with BackgroundWorker, so maybe you could do something like this to let the other thread wait.

public void Populate(bool reload)
{
    while (this.DataWorker.IsBusy) {
     Thread.Sleep(100);
    }

    // Disable the filter options
    IvdSession.Instance.FilterManager.SetEnabledState(this.GetType(), false);

    // Perform the population
    this.DataWorker.RunWorkerAsync(reload);
}

Answer 2

First, add a lock around your Populate method body:

private object _exclusiveAccessLock = new object();
public void Populate(bool reload)
{
    lock (_exclusiveAccessLock)
    {
         // start the job
    }
}

This will help you avoid a race condition (although: if I got it right, since you are using a Windows.Forms Timer, it will always fire from the Gui thread, so they should never be executed exactly at the same time).

Next, I am not sure if you should throw the exception at all. You can, for example, set an additional flag that shows you that the worker hasn't finished yet, but that is what IsBusy should tell you anyway.

Then there is the m_BlockFilter flag. I cannot see where you are setting it from. It should be set inside the lock also, not in the background thread, because in that case you cannot be sure that it will not be delayed. You also need to make the field volatile if you are going to use it as a cross-thread flag.