BindingList.Add() doesn't work cross thread even with lock

Question

I am just learn C#/.NET and I met this problem.

So in my solution i have 2 projects: winforms UI and dll with logic. In dll i have BindingList which provides datasource for listBox in UI.

UI:

public partial class Form1 : Form
{
    private Class1 _class1;

    public Form1()
    {
        InitializeComponent();
        _class1 = new Class1(); // logic class insatce
        listBox1.DataSource = _class1.BindingList;
    }

    private void button1_Click(object sender, EventArgs e)
    {
        _class1.Add();
    }

    private void button2_Click(object sender, EventArgs e)
    {
         _class1.Remove();
    }
}

Logic class:

public class Class1
{
    public BindingList<string> BindingList { get; set; } = new BindingList<string>() ;

    public void Add()
    {
        var th = new Thread(() =>
        {
            lock (BindingList)
            {
                BindingList.Add("1");
            }
        }) {IsBackground = true};
        th.Start();
        // works fine
        //BindingList.Add("1");
    }

    public void Remove()
    {
        if (BindingList.Count > 1)
        {
            BindingList.RemoveAt(0);
        }
    }
}

So the problem that if I just Run solution(ctrl + F5) all works fine, but in debug mod(F5) nothing happens when i press button. All answers I found say : "use lock" so i used lock and listbox still not react on adding elements to list. Please help me what i am doing wrong or where i missed something.

PS sorry for my English.

Answer 1

First, to be clear: you may or may not need to use lock here. That would depend on whether there are actually two or more threads accessing the BindingList<T> object concurrently, i.e. literally at the same time (e.g. two or more threads adding items to the list, or one thread adding items while another one is trying to read from the list). In your code example, this does not appear to be the case, and so it wouldn't be necessary. Regardless, the lock statement does something completely different than what is required to address the specific issue you're asking about, and in any case only works when threads use lock cooperatively on the same object (if only one thread calls lock, that doesn't help).

The basic issue is that the ListBox can't respond to events from BindingList when those events are raised on other than the UI thread. Typically, the solution to this would be to call Control.Invoke() or similar to execute the list-modifying operation in the UI thread. But in your case, the class that owns the BindingList isn't a UI object and so doesn't naturally have access to the Control.Invoke() method.

IMHO, the best solution preserves the UI thread knowledge in the UI object(s) involved. But doing it this way would require having the Class1 object hand over at least some of the control over the list to that UI object. One such approach would involve adding an event to the Class1 object:

public class AddItemEventArgs<T> : EventArgs
{
    public T Item { get; private set; }

    public AddItemEventArgs(T item)
    {
        Item = item;
    }
}

public class Class1
{
    public EventHandler<AddItemEventArgs<string>> AddItem;

    public BindingList<string> BindingList { get; set; }

    public Class1()
    {
        // Sorry, old-style because I'm not using C# 6 yet
        BindingList = new BindingList<string>();
    }

    // For testing, I prefer unique list items
    private int _index;

    public void Add()
    {
        var th = new Thread(() =>
        {
            string item = (++_index).ToString();

            OnAddItem(item);
        }) { IsBackground = true };
        th.Start();
    }

    public void Remove()
    {
        if (BindingList.Count > 1)
        {
            BindingList.RemoveAt(0);
        }
    }

    private void OnAddItem(string item)
    {
        EventHandler<AddItemEventArgs<string>> handler = AddItem;

        if (handler != null)
        {
            handler(this, new AddItemEventArgs<string>(item));
        }
    }
}

Then in your Form1:

public partial class Form1 : Form
{
    private Class1 _class1;

    public Form1()
    {
        InitializeComponent();
        _class1 = new Class1(); // logic class instance
        _class1.AddItem += (sender, e) =>
        {
            Invoke((MethodInvoker)(() => _class1.BindingList.Add(e.Item)));
        };
        listBox1.DataSource = _class1.BindingList;
    }

    private void button1_Click(object sender, EventArgs e)
    {
        _class1.Add();
    }

    private void button2_Click(object sender, EventArgs e)
    {
        _class1.Remove();
    }
}

A variation on this theme would be to have two different "add" methods in Class1. The first would be the one you have now, which ultimately uses a thread. The second would be the one that would be required to be called from the UI thread, and which would actually add the item. In the AddItem event handler in the form, instead of adding the item directly to the list, the second "add" method would be called to do that for the form.

Which is best depends on how much abstraction you want in your Class1. If you're trying to hide the list and its operations from other classes, then the variation would be better. But if you don't mind updating the list from somewhere other than in the Class1 code, the code example above should be fine.

An alternative is to make your Class1 object thread-aware, similar to how e.g. BackgroundWorker works. You do this by capturing the current SynchronizationContext for the thread when the Class1 object is created (on the assumption that the Class1 object is created in the thread where you want to return to, to add an item). Then when adding an item, you use that context object for the add.

That looks like this:

public class Class1
{
    public BindingList<string> BindingList { get; set; }

    private readonly SynchronizationContext _context = SynchronizationContext.Current;

    public Class1()
    {
        BindingList = new BindingList<string>();
    }

    private int _index;

    public void Add()
    {
        var th = new Thread(() =>
        {
            string item = (++_index).ToString();

            _context.Send(o => BindingList.Add(item), null);
        }) { IsBackground = true };
        th.Start();
    }

    public void Remove()
    {
        if (BindingList.Count > 1)
        {
            BindingList.RemoveAt(0);
        }
    }
}

In this version, no changes to Form1 are needed.

There are lots of variations on this basic scheme, including some which put the logic into a specialized BindingList<T> subclass instead. For example (to name a couple):
Cross-Thread Form Binding - Can it be done?
BindingList<> ListChanged event

Finally, if you want to really hack things together, you can just force the entire binding to reset any time the list has changed. In that case, you would not need to change Class1, but you would need to change Form1:

public partial class Form1 : Form
{
    private Class1 _class1;

    public Form1()
    {
        bool adding = false;

        InitializeComponent();
        _class1 = new Class1(); // logic class instance
        _class1.BindingList.ListChanged += (sender, e) =>
        {
            Invoke((MethodInvoker)(() =>
            {
                if (e.ListChangedType == ListChangedType.ItemAdded && !adding)
                {
                    // Remove and re-insert newly added item, but on the UI thread
                    string value = _class1.BindingList[e.NewIndex];

                    _class1.BindingList.RemoveAt(e.NewIndex);
                    adding = true;
                    _class1.BindingList.Insert(e.NewIndex, value);
                    adding = false;
                }
            }));
        };
        listBox1.DataSource = _class1.BindingList;
    }

    // ...
}

I don't really advise this approach. But if you have no way to change Class1, it's about the best you can do.