Threading problem with passing data to a thread from within a DragDrop event

I have a C# app with a button for dragging and dropping files. I am able to take 6 files from my desktop and drop it onto the button and have it process those 6 files.

However, when I start a thread from the DragDrop event and pass the file path to a new thread started from within the DragDrop event, the file path is incorrect once the thread receives the FilePath parameter.

If I execute my code by dragging 6 text files onto my button (I had to strip a lot of code out of it for this example), I will see the following in my console:

++ Calling testthread with these params: false, TestButton,test.txt,c:\test.txt

++ Calling testthread with these params: false, TestButton,test2.txt,c:\test2.txt

++ Calling testthread with these params: false, TestButton,test3.txt,c:\test3.txt

++ Calling testthread with these params: false, TestButton,test4.txt,c:\test4.txt

++ Calling testthread with these params: false, TestButton,test5.txt,c:\test5.txt

++ Calling testthread with these params: false, TestButton,test6.txt,c:\test6.txt

The above output is correct

The following output is incorrect, notice the FilePath does not match the CleanFileName like it does in the above console output.

++ 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

++ 开发者_C百科testthread Thread - CallingfromPendingUploads == false ButtonName == TestButton CleanFileName == test6.txt FilePath = c:\test5.txt

As you can see, the FilePath from thread does not match the FilePath that is being passed to the Thread before it starts. All of the FilePaths are off when compared to the filename that is passed to the Thread. And some of the FilePaths are duplicates such as text5.txt.

I have been struggling with this for hours. 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);
}

All of your threads are sharing the same file variable.
If one of the threads only starts running after the UI 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 will get its own variable.

For example:

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

Since the file variable is now scoped within the loop, each iteration gets a separate variable.

this will fix it:

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

You've fallen into the common trap of lambdas and loop variables, the threading just made it obvious.

When you create a lambda, any variables you use will be closed over by reference not by value as you may have assumed.

What this means is that when you do:

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

The lambda you've created closed over outer, whose reference remains the same at each loop iteration even though it's value may 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 threads into the mix, you've included a fixed reference to a variable whose value is being changed on a different thread. Hence file appeared to jump 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 over locally at each loop iteration. You need to follow a similar strategy to correct your usage 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));
}

I suspect that the value of file may get overwritten in this line: -- ( at least I was correct here )

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

Edit -- I concur that @SLaks answer is correct and I understand why. I also understand why my answer is incorrect. Instead of deleting it, I believe it has value in demonstrating why a lock won't work in this case. And, for that reason, I am making it CW.

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

I DON'T think you would 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);
    }
}