Cross-thread operation not valid: Control 'listBox1' accessed from a thread other than the thread it was created on

Question

I want to add the items to the listBox1 in Modbus_request_event.I have gone through the solution provided for this problem and modified the code with MethodInvoker Delegate and still it will not add the items to listBox1. Here is my code

     public void Modbus_Request_Event(object sender, ModbusSlaveRequestEventArgs e)
        {
            //disassemble packet from master
            byte fc = e.Message.FunctionCode;
            byte[] data = e.Message.MessageFrame;
            byte[] byteStartAddress = new byte[] { data[3], data[2] };
            byte[] byteNum = new byte[] { data[5], data[4] };
            short StartAddress = BitConverter.ToInt16(byteStartAddress, 0);
            short NumOfPoint = BitConverter.ToInt16(byteNum, 0);


            string fc1 = Convert.ToString(fc);
           string StartAddress1 = Convert.ToString(StartAddress);
           string NumOfPoints1 = Convert.ToString(NumOfPoint);

           /*Adds the items to listBox1*/
            Invoke(new MethodInvoker(delegate () { listBox1.Items.Add(fc1); listBox1.Items.Add(StartAddress1); listBox1.Items.Add(NumOfPoints1); }));
//it runs infinitely not able to add to listbox//


        }

can anyone please help me to solve this?

Possible duplicate of [Cross-thread operation not valid: Control 'textBox1' accessed from a thread other than the thread it was created on](https://stackoverflow.com/questions/10775367/cross-thread-operation-not-valid-control-textbox1-accessed-from-a-thread-othe) — Crowcoder, Oct 03 '18 at 12:13
This isn't a duplicate.. read it carefully (check the code)... The `Modbus_Request_Event` is a method of your form? — Jeroen van Langen, Oct 03 '18 at 12:42
Modbus_Request_Event method will be invoked when the Button in the form is clicked by the user to accept the request from Modbus Master.Here in this method after disassemble the packet from the master's request I want to add few of those variables to the listBox1.So can anyone please help me to solve this? — pooja, Oct 03 '18 at 12:59
@J.vanLangen it *is* a duplicate. Such an exception is only thrown when trying to modify the UI from another thread. Somehow, somewhere, the OP is trying to modify the UI from a different thread. — Panagiotis Kanavos, Oct 03 '18 at 13:12
@pooja have you tried debugging the application? What does the *full* exception look like? `Modbus_Request_Event` is definitely *not* a button click event handler. If it's called by Modbus, it's probably running on another thread and can't modify the UI. `Invoke(new MethodInvoker` is essentially a no-op. If you wanted the delegate to run on the UI thread you should have used `listBox1.Invoke` or better yet `listBox1.BeginInvoke`. — Panagiotis Kanavos, Oct 03 '18 at 13:15
A far better option though would be to use `Progress` [as shown here](https://blogs.msdn.microsoft.com/dotnet/2012/06/06/async-in-4-5-enabling-progress-and-cancellation-in-async-apis/) and decouple the Modbus event handler from the UI — Panagiotis Kanavos, Oct 03 '18 at 13:17
Another very good option is to use a TaskCompletionSource to convert that event to a Task and use `async/await` in your code [as shown here](https://learn.microsoft.com/en-us/dotnet/standard/asynchronous-programming-patterns/interop-with-other-asynchronous-patterns-and-types#EAP) — Panagiotis Kanavos, Oct 03 '18 at 13:20
Invoke() is quite dangerous and almost never correct. Only truly required when the return value is necessary and that's a race waiting to happen. Strongly favor BeginInvoke. And use the debugger's Debug > Windows > Threads to find out why this deadlock occurred. The main thread is doing something it should not be doing. If that doesn't pan out either then add this.Show(), you might see a second instance of the form pop up. — Hans Passant, Oct 03 '18 at 14:47

score 1 · Answer 1 · answered Oct 03 '18 at 13:46

First of all, the duplicate is a duplicate. You can't modify the UI from another thread. Before .NET 4.5, people would use Invoke or BeginInvoke on a control to marshal a delegate to the UI thread and run it there. The question's code though calls Invoke() by itself, essentially running the delegate on the thread it's currently on.

In short this:

Invoke(new MethodInvoker(delegate () { listBox1.Items.Add(fc1); listBox1.Items.Add(StartAddress1); listBox1.Items.Add(NumOfPoints1); }));

Is essentially the same as this, as far as threading is concerned :

listBox1.Items.Add(fc1); listBox1.Items.Add(StartAddress1); listBox1.Items.Add(NumOfPoints1);

With .NET 4.5 and later, you can use Progress to report progress from any thread or task, as shown in Enabling Progress and Cancellation in Async APIs. Given that the earliest supported .NET version is 4.5.2, you can assume that class is available everywhere.

By using the Progress<T> and the IProgress<T> interface you can decouple the event from the UI, which means you can handle the data any way you want, even on different forms. You could move the Modbus class to a different class or library to keep it separate from the UI.

In the simplest case, you can instantiate the Progress<T> class in your form's constructor and call it through the IProgress<T> interface from the event handler, eg :

public class ModbusData
{
    public byte Fc {get; set;}
    public short StartAddress {get; set;}
    public short NumOfPoints {get; set;}
}

public class MyForm : ...
{

    IProgress<ModbusData> _modbusProgress;

    public MyForm()
    {
        __modbusProgress=new Progress<ModbusData>(ReportProgress);
    }

    public void ReportProgress(ModbusData data)
    {
        listBox1.Items.Add(data.fc1.ToString()); 
        listBox1.Items.Add(dta.StartAddress1.ToString()); 
        listBox1.Items.Add(data.NumOfPoints1.ToString()); 
    }

And report progress from the event, no matter which thread it's raised on :

    public void Modbus_Request_Event(object sender, ModbusSlaveRequestEventArgs e)
    {
        //disassemble packet from master
        byte fc = e.Message.FunctionCode;
        byte[] data = e.Message.MessageFrame;
        byte[] byteStartAddress = new byte[] { data[3], data[2] };
        byte[] byteNum = new byte[] { data[5], data[4] };
        short StartAddress = BitConverter.ToInt16(byteStartAddress, 0);
        short NumOfPoint = BitConverter.ToInt16(byteNum, 0);

        var modData = new ModbusData {
                 Fc = fc,
                 StartAddress = StartAddress,
                 NumOfPoints = NumOfPoint
        };
       _progress.Report(modData);
    }

If you decide to move the Modbus classes to another class, all you have to do is pass an IProgress<ModbusData> instance to them before you start using them.

For example :

class MyModbusController
{
    IProgress<ModbusData> _modbusProgress;

    public MyModbusController(IProgress<ModbusData> progress)
    {
        _modbusProgress=progress;
    }

    public void Modbus_Request_Event(...)
}

This means that if the modbus raises 1000 events per second, you ui thread will receive 1000 callbacks a second. The `Progress` just invokes the synchronization context. GF ui response. — Jeroen van Langen, Oct 04 '18 at 07:37
@J.vanLangen that was neither the question nor a real problem. There are a *lot* of ways to batch updates. In fact, if one used data binding instead of directly modifying the UI, updating the UI is a lot simpler - just wait a bit before raising the NotifyChanged event — Panagiotis Kanavos, Oct 04 '18 at 07:38

Jeroen van Langen · Answer 2 · 2018-10-04T07:46:16.290

This doesn't fix your problem.., because your code doesn't look wrong, but this might help making better solutions about threading and gui.

You are using Invoke to synchronize the modbus thread and the gui-thread. This means when an event of the modbus is received, you add items to the listbox. When it is invoking the gui-thread, your modbus thread is blocked. So the speed of your modbus communication is dependend on the speed of the gui components. Try to avoid such nasty blocks. If you receive many modbus events per second, your listbox will be flooded with repaints and your application will be throttled.

How to fix this?:

You should add the items to a list (concurrentcollection) and use a timer to update your gui. This way you can determine when the received events are added (in a batch) to the listbox. A timer with an interval of 100ms is enough.

Pseudo example code:

public class Data
{
    public string Fc1 {get; set;}
    public string StartAddress1 {get; set;}
    public string NumOfPoints1 {get; set;}
}

ConcurrentQueue<Data> _modbusEvents = new ConcurrentQueue<Data>();


public void Modbus_Request_Event(object sender, ModbusSlaveRequestEventArgs e)
{
    //disassemble packet from master
    byte fc = e.Message.FunctionCode;
    byte[] data = e.Message.MessageFrame;
    byte[] byteStartAddress = new byte[] { data[3], data[2] };
    byte[] byteNum = new byte[] { data[5], data[4] };
    short StartAddress = BitConverter.ToInt16(byteStartAddress, 0);
    short NumOfPoint = BitConverter.ToInt16(byteNum, 0);


    string fc1 = Convert.ToString(fc);
    string StartAddress1 = Convert.ToString(StartAddress);
    string NumOfPoints1 = Convert.ToString(NumOfPoint);

    // add to the concurrentqueue
    _modbusEvents.Add(new Data { Fc1 = fc1, StartAddress1 = StartAddress1, NumOfPoints1 = NumOfPoints1 });

   /*Adds the items to listBox1*/
    //Invoke(new MethodInvoker(delegate () { listBox1.Items.Add(fc1); listBox1.Items.Add(StartAddress1); listBox1.Items.Add(NumOfPoints1); }));
//it runs infinitely not able to add to listbox//

}

private void Timer_Tick(object sender, EventArgs e)
{
    if(_modbusEvents.Count > 0)
    {
        listBox1.BeginUpdate();

        while(_modbusEvents.TryDequeue(out var result))
        {
             listBox1.Items.Add(result.Fc1);
             listBox1.Items.Add(result.StartAddress1);
             listBox1.Items.Add(result.NumOfPoints1);
        }

        listBox1.EndUpdate();
    }
}

You don't need a concurrentbag to update the UI thread. In fact, a ConcurrentBag has a high cross-thread marshalling cost in this case. .NET itself implements cross-thread progress reporting with the `Progress` class — Panagiotis Kanavos, Oct 03 '18 at 13:24
Never heard of Progress, I'll look into it. I'd normally use a simple lock for it. — Jeroen van Langen, Oct 04 '18 at 07:26
Locks don't report progress nor do they send data to other threads — Panagiotis Kanavos, Oct 04 '18 at 07:27
Why would you think a lock would send any data? That's ridiculous. I would use a lock in combination with the solution above — Jeroen van Langen, Oct 04 '18 at 07:32
You compared IProgress and locks. They aren't the same at all. As for this solution it *shouldn't* need locks. It should use ConcurrentQueue, not ConcurrentBag, since ConcurrentBag uses thread-local storage. It shouldn't need a timer either - the timer is needed here because the concurrent collections don't have awaitable operations or notifications like eg an ObservableCollection. If one wanted to run an action in response to a new item, there's ActionBlock. — Panagiotis Kanavos, Oct 04 '18 at 07:36
A concurrentqueue might be better. thanks for that. I'd rather use a timer to check the queue instead for firing zilions of events to the synchronizationcontext — Jeroen van Langen, Oct 04 '18 at 07:39
Again, not the question and there are better ways to do this if it becomes a problem. Like *not* sending an event for every single input perhaps? If you care about processing events, there are Dataflow blocks, Channels, Reactive Extensions — Panagiotis Kanavos, Oct 04 '18 at 07:43

score -1 · Answer 3 · edited Oct 03 '18 at 15:47

-1

Try using the following code:

Invoke((MethodInvoker)delegate 
{
listBox1.Items.Add(fc1);
listBox1.Items.Add(StartAddress1);
listBox1.Items.Add(NumOfPoints1);
});

edited Oct 03 '18 at 15:47

Hadi

36,233
13
65
124

answered Oct 03 '18 at 15:11

trksyln

136
10

Cross-thread operation not valid: Control 'listBox1' accessed from a thread other than the thread it was created on

3 Answers3