Threading issue with passing data to a stream from a DragDrop event

Question

I have a C # application with a button to drag and drop files. I can take 6 files from my desktop and drop them on the button and process these 6 files.

However, when I start the stream from the DragDrop event and pass the path to the new stream started from the DragDrop event, the file path is incorrect after the stream receives the FilePath parameter.

If I executed my code by dragging 6 text files onto my button (I had to remove a lot of code from it for this example), I will see the following on my console:

++ Call testthread with these parameters: false, TestButton, test.txt, c: \ test.txt
++ Call testthread with these parameters: false, TestButton, test2.txt, c: \ test2.txt
++ Call testthread with these parameters: false, TestButton, test3.txt, c: \ test3.txt
++ Call testthread with these parameters: false, TestButton, test4.txt, c: \ test4.txt
++ Call testthread with these parameters: false, TestButton, test5.txt, c: \ test5.txt
++ Call testthread with these parameters: false, TestButton, test6.txt, c: \ test6.txt

The above conclusion is correct -

The following conclusion is incorrect, note that FilePath does not match the name CleanFileName, as is done in the above console release.

++ testthread Thread - CallingfromPendingUploads == false ButtonName == TestButton CleanFileName == test.txt FilePath = c: \ test2.txt
++ testthread Thread - CallingfromPendingUploads == false ButtonName == TestButton CleanFileName == test1.txt FilePath = c: \ test3.txt
++ testthread Thread - CallingfromPendingUploads == false ButtonName == TestButton CleanFileName == test3.txt FilePath = c: \ test4.txt
++ testthread Thread - CallingfromPendingUploads == false ButtonName == TestButton CleanFileName == test4.txt FilePath = c: \ test5.txt
++ testthread Thread - CallingfromPendingUploads == false ButtonName == TestButton CleanFileName == test5.txt FilePath = c: \ test5.txt
++ testthread Thread - CallingfromPendingUploads == false ButtonName == TestButton CleanFileName == test6.txt FilePath = c: \ test5.txt

As you can see, the FilePath from the stream does not match the FilePath that is passed to Thread before it starts. All FilePaths files are disabled compared to the file name that is passed to Thread. And some of the FilePaths are duplicates, such as text5.txt.

I struggled with this watch. Can someone please tell me what I am doing wrong?

private void btnClick_DragDrop(object sender, DragEventArgs e) { string[] file = (string[])e.Data.GetData(DataFormats.FileDrop); string ButtonName = "TestButton" string[] files = new string[10]; files = (string[])e.Data.GetData(DataFormats.FileDrop); foreach (string file in files) { FileInfo fileInfo = new FileInfo(file); Console.WriteLine("++ Filename: " + fileInfo.Name + " Date of file: " + fileInfo.CreationTime + " Type of file: " + fileInfo.Extension + " Size of file: " + fileInfo.Length.ToString()); string CleanFileName = System.Web.HttpUtility.UrlEncode(fileInfo.Name.ToString()); //Start thread try { Console.WriteLine("++ Calling testthread with these params: false, " + ButtonName + "," + CleanFileName + "," + file); new Thread(() => testthread(false, ButtonName, CleanFileName, file)).Start(); Console.WriteLine("++ testthead thread started @ " + DateTime.Now); } catch (Exception ipwse) { logger.Debug(ipwse.Message + " " + ipwse.StackTrace); } } } public void testthread(bool CalledfromPendingUploads, string ButtonName, string CleanFileName, string FilePath) { Console.WriteLine("++ testthread Thread - CallingfromPendingUploads == " + CalledfromPendingUploads.ToString() + " ButtonName == " + ButtonName + " CleanFileName == " + CleanFileName + " FilePath = " + FilePath); } 

Answer 1

All your threads share the same file variable.
If one of the threads starts only after the user interface thread has started the next iteration, it will use the next value of the file variable.

You need to declare a separate variable inside the loop so that each thread gets its own variable.

For instance:

 foreach (string dontUse in files) { string file = dontUse; ... } 

Since the file variable is now included in the loop, each iteration gets a separate variable.

Answer 2

this will fix this:

 string tempFile = file; new Thread(() => testthread(false, ButtonName, CleanFileName, tempFile)).Start(); 

Answer 3

You fall into the common trap of lambdas and loop variables, threads just made it obvious.

When creating a lambda, any variables you use will be closed by reference not by value, as you might have expected.

This means that when you do:

 foreach (var outer in collection) { var state = 42; Grok(() => frob(outer, state)); } 

The lambda you created is closed over outer , the link of which remains unchanged at each iteration of the loop, even if this value can change!

 // Conceptual look at the previous code Bar outer; // outside the loop-scope foreach (outer in collection) { var state = 42; // inside the loop-scope Grok(() => frob(outer, state)); } 

Therefore, when you introduce streams into the mix, you include a fixed reference to a variable whose value changes in another stream. Consequently, file appeared to go to the last value when your threads slowed down.

In the case of CleanFileName it was declared inside the loop, and thus it was closed locally at each iteration of the loop. You should follow a similar strategy to fix the use of file :

 foreach (var outer in collection) { var inner = outer; // make a closure safe copy of the loop variable var state = 42; Grok(() => frob(inner, state)); } 

Answer 4

I suspect that the file value may be overwritten on this line: - ( at least I was right here )

 new Thread(() => testthread(false, ButtonName, CleanFileName, file)).Start(); 

Edit - I agree that @SLaks answer is correct, and I understand why. I also understand why my answer is incorrect. Instead of deleting it, I believe that it matters, demonstrating why in this case the lock will not work. And for this reason I do this CW.

Maybe locking is NOT necessary with a modification in the line of the above code.

I DO NOT think you need something close to this:

 object key = new object(); private void btnClick_DragDrop(object sender, DragEventArgs e) { // your code ... //Start thread try { Console.WriteLine("++ Calling testthread with these params: false, " + ButtonName + "," + CleanFileName + "," + file); lock (key) { string[] fileCopy; file.CopyTo(fileCopy); new Thread(() => testthread(false, ButtonName, CleanFileName, fileCopy)).Start(); } Console.WriteLine("++ testthead thread started @ " + DateTime.Now); } catch (Exception ipwse) { logger.Debug(ipwse.Message + " " + ipwse.StackTrace); } }