2

The following code works fine without multithreading. But if I use threading, it fails. If I choose more than one item in the checkedListBox, the first will be ignored and the others are random ...

I think there is a problem with committing the data. What do you think?

 private void sendCom(String com) { //send command to selected item int i=0; String IP; foreach (var item in checkedListBox1.CheckedItems) { Console.WriteLine(item.ToString()); IP = item.ToString(); theThreads[i] = new Thread(new ThreadStart(() => sendComThread(IP, com) )); theThreads[i].Start(); //sendCom(IP, com); i++; } } private void sendComThread(String IP, String com) { // send an command System.Console.WriteLine(IP + com); }
Improve this question
0

4 Answers 4

Reset to default
4

The fundamental problem is that your variable capture is capturing a single variable which is then shared between all threads. So each time a thread reads the shared variable, it gets whatever value happens to have been put in there most recently. As well as that semantic error, there is a clear data races on the shared variable.

The simplest solution is to create one variable per thread. Just move the variable's declaration inside the loop. Like this:

foreach (var item in checkedListBox1.CheckedItems) { .... String IP = item.ToString(); //NB variable declared inside loop theThreads[i] = new Thread(new ThreadStart(() => sendComThread(IP, com) )); .... }

Now each thread has its own private instance of the string variable.

Improve this answer
Sign up to request clarification or add additional context in comments.

3 Comments

@tux007 The subtle difference here is that IP is declared inside the scope of the loop, rather than outside. As such, a new object is created per iteration, and passed to the lambda. When it's declared outside of the loop, each iteration writes to the same object, causing the problem you encountered.
Also +1 because you taught me something new today. I always assumed that loop variables had to be copied local to the thread, but I was clearly wrong. That'll save me resorting to helper classes and anonymous types! :)
okay, I tried, and it works as good as the version with the struct.
2

The problem here is that your thread is reading the state of the loop from the lambda expression, within the new thread, rather than passing the actual values over to the thread.

This means that by the time the new thread gets scheduled on the CPU, the loop has actually iterated forward to an unknown state. This is why your values appear random.

Here's what's happening in a step by step:

  1. The () => sendComThread(IP, com) lambda is created, which references the two parameters.
  2. theThreads[i].Start(); is called, but this does not guarantee that the code in that thread will immediately run. It is likely that the current code will continue for a while before the thread scheduler on the system switches the context to a different thread.
  3. The next loop iteration occurs and IP = item.ToString(); is executed, changing the value of IP. This may occur more than once.
  4. The context switch occurs on the processor and the other thread is executed, or the other thread executes on another processor (core), reading the reference to IP from the lambda expression.
  5. This causes a cross-thread read, meaning that the state of IP is undefined.

The solution is to pass the values over during thread creation, so that they are copied local to the thread:

struct SendComThreadParams { public string IP; public string Com; public SendComThreadParams(string ip, string com) { this.IP = ip; this.Com = com; } } private void sendCom(String com) { //send command to selected item int i=0; String IP; foreach (var item in checkedListBox1.CheckedItems) { Console.WriteLine(item.ToString()); IP = item.ToString(); theThreads[i] = new Thread(new ParameterizedThreadStart(sendComThread)); theThreads[i].Start(new SendComThreadParams(IP, com)); i++; } } private void sendComThread(object threadParam) { var p = (SendComThreadParams)threadParam; // send an command System.Console.WriteLine(p.IP + p.Com); }

This properly copies the parameters over to the thread, such that their values are guaranteed to be in a defined state.

Improve this answer

8 Comments

thanks, it works fine! By the way, can you give me an example with threadpools?
Or you could just use locals that are defined inside the loop. No need to create a class for this.
This does seem like overkill. Also, what do you mean by that cross-thread read point. That looks wrong to me.
@DavidHeffernan I'm saying that the loop executes on Thread 0, and the reads execute on threads 1, 2, 3, ..., etc. The threads doing the reads are accessing an object that is being actively written to by another thread, hence a cross-thread read of an object that is in an undefined state.
That's a data race. How does that manifest in .net? Is cross thread read the .net name for a race?
|
1

theThreads[i].Start() will not run the new thread immediately and the IP variable may change in the mean time.

defining the IP variable inside the for loop will fix the problem:

string IP = item.ToString();
Improve this answer

9 Comments

No, it won't, because that will simply generate a cross-thread read on the current object that the iterator is referencing, leading to the same problem. Except it might possibly be worse, because it could cause a cross-thread operation exception due to interacting with the UI.
@Polynomial No, if you have a local inside a loop, then each iteration of the loop creates the local anew. So, there is no sharing between the threads. And I'm not sure what does this have to do with the UI.
@svick Still horrible practice. What if the method returns before the newly created thread runs? The locals are all disposed of at that point. A garbage collection cycle would likely cause similar problems.
@polynomial Not so. They are captured by the lambda which extends their life.
@Amine I upvoted you since your answer is fine. The downvote seems wrong to me. I added an answer myself to try and expand and explain a bit more.
|
0

Here another version of Polynomials code. this time with threadpools 

struct SendComThreadParams { public string IP; public string Com; public SendComThreadParams(string ip, string com) { this.IP = ip; this.Com = com; } } private void sendCom(String com) { //send command to selected item int i=0; String IP; foreach (var item in checkedListBox1.CheckedItems) { Console.WriteLine(item.ToString()); IP = item.ToString(); ThreadPool.QueueUserWorkItem(new WaitCallback(sendComThread), (object)new SendComThreadParams(IP, com)); i++; } } private void sendComThread(object threadParam) { var p = (SendComThreadParams)threadParam; // send an command System.Console.WriteLine(p.IP + p.Com); }
Improve this answer

3 Comments

What does this add. Thread pools are a side issue. The issue relates to variable capture. I hope you are reading the various comments.
this add the option of threadpool. I ask in a coment if someone could give me an example. I got it by mayself and posted the answer.
Answers are supposed to address the question asked. Threadpool variants are only really pertinent to you. I think this answer adds little and just gets in the way of an interesting topic.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.