How do I keep parameters from being mixed up using Multithreading?

[Guest]

Please see the code below as I am trying to use multithreading for copying
files to a new location in a way that improves performance of the client
windows application.

The problem occurs when 2 or more threads are created, the ImportOneFile
method attempts to add a previously added file. If I allow 4 maximum threads
and process 4 files, the last file is attempted 4 times and none of the other
files are added to the destination. If I reduce maximum threads to 2, the
second file is attempted twice, the first one is forgotten and other files
are added to the destination.

Anyone have any idea why the values passed to ImportOneFile are sometimes
"forgotten"? I thought the lock statement would protect against this.

Thanks in Advance, Mark

Sample code:
private void ImportFiles(string sourceFolderPath)
{
int numberOfRunningThreads;
int maxNumberThreads = 4;

//navigate through all of the files in this folder
FileInfo[] currentFolderFiles;
currentFolderFiles = new DirectoryInfo(sourceFolderPath)).GetFiles();

foreach (FileInfo currentFile in currentFolderFiles)
{

//make sure that we don’t exceed the number of threads specified
while (numberOfRunningThreads > (maxNumberThreads - 1))
{
//wait
Thread.Sleep(500);
}

//create a string array to pass to ImportFile
string[] importDetails = new string[2];

//set the array to the file name and the doc library path
importDetails[0] = currentFile.FullName;
importDetails[1] = destinationPath;

//assign the method to the callback to the ImportOneFile method
WaitCallback callBack = new WaitCallback(ImportOneFile);

//interlock increment the number of threads running
Interlocked.Increment(ref _numberOfRunningThreads);

//create a thread on the queue and provide the method with the
string array
ThreadPool.QueueUserWorkItem(callBack, (object)importDetails);
} //end of for loop

//make sure all of the threads are completed processing before leaving method
int loopCount = 0;
while ((_numberOfRunningThreads > 0) && (loopCount < 6000))
{
Thread.Sleep(50);
loopCount++;
}

if (loopCount > 5999)
{
throw new Exception(“Timeout waiting for threads to complete.â€);
}


}

private void ImportOneFile(object fileDetails)
{
//implement thread locking for protecting while using multithreading
lock(this)
{
//Using QueueUserWorkItem, parameters have to be sent to an object
// so here the parameters are repackaged in a string
string[] detailStrings = (string[])fileDetails;

//get an implementation of the file adder from the factory
CustomFileAdder fileAdder = new CustomFileAdder;

//add file, provide destination path, file name, contents
of file
fileAdder.AddFile(detailStrings[1],
Path.GetFileName(detailStrings[0]), BinaryFileContents(detailStrings[0]));
}
//interlock decrement since we are done
Interlocked.Decrement(ref _numberOfRunningThreads);

}

 

[Mattias Sjögren]

Please see the code below as I am trying to use multithreading for copying
files to a new location in a way that improves performance of the client
windows application.

Please post relevant parts of your actual code that compiles. The code
you posted have a number of errors in it, and it's pointless to guess
what's wrong if we're not looking at your actual code.

I thought the lock statement would protect against this.

By using lock(this) (which is a bad thing in itself) around the entire
ImportOneFile method body you're effectively serializing access to
that code - only one thread at a time will run it. So the point of
having multiple threads here is sort of lost.


Mattias

 

[Guest]

Thanks for responding, Mattias!

Sorry about the code. Let me know if you have any issues with this:

//you will need the following references
using System.IO;
using System.Threading;

//add the following methods to a class, calling ImportFiles
private void ImportFiles()
{

//create a string array to pass to ImportFile
string[] importDetails = new string[2];

int numberOfRunningThreads = 0;
int maxNumberThreads = 4;

string sourceFolderPath = "E:\\Data\\TestUploadDocs";
string destinationPath = "E:\\Data\\Destination";

//navigate through all of the files in this folder
FileInfo[] currentFolderFiles;
currentFolderFiles = new DirectoryInfo(sourceFolderPath)).GetFiles();


foreach (FileInfo currentFile in currentFolderFiles)
{

//make sure that we don’t exceed the number of threads specified
while (numberOfRunningThreads > (maxNumberThreads - 1))
{
//wait
Thread.Sleep(500);
}

//set the array to the file name and the doc library path
importDetails[0] = currentFile.FullName;
importDetails[1] = destinationPath;

//assign the method to the callback to the ImportOneFile method
WaitCallback callBack = new WaitCallback(ImportOneFile);

//interlock increment the number of threads running
Interlocked.Increment(ref _numberOfRunningThreads);

//create a thread on the queue and provide the method with the
string array
ThreadPool.QueueUserWorkItem(callBack, (object)importDetails);
}

//make sure all of the threads are completed processing before leaving method
int loopCount = 0;
while ((_numberOfRunningThreads > 0) && (loopCount < 6000))
{
Thread.Sleep(50);
loopCount++;
}

if (loopCount > 5999)
{
throw new Exception("Timeout waiting for threads to complete.");
}


}

private void ImportOneFile(object fileDetails)
{
//implement thread locking for protecting while using multithreading
lock(this)
{
//Using QueueUserWorkItem, parameters have to be sent to an object
// so here the parameters are repackaged in a string
string[] detailStrings = (string[])fileDetails;


Console.WriteLine("Inside ImportOneFile the file is {0} and the
destination path is {1}", detailStrings[0], detailStrings[1]);
}
//interlock decrement since we are done
Interlocked.Decrement(ref _numberOfRunningThreads);

}

 

[Mattias Sjögren]

private void ImportFiles()
{

//create a string array to pass to ImportFile
string[] importDetails = new string[2];

Here (unlike your first post) you're creating the array once outside
the foreach loop. By reusing the same array all the time you're likely
to see the behavior you initially described. Moving the array inside
the foreach loop should fix it.

currentFolderFiles = new DirectoryInfo(sourceFolderPath)).GetFiles();

You still have unbalanced parentheses here.

//make sure that we don’t exceed the number of threads specified
while (numberOfRunningThreads > (maxNumberThreads - 1))
{
//wait
Thread.Sleep(500);
}

This will never run since the local numberOfRunningThreads variable is
never incremented, only the _numberOfRunningThreads field.

lock(this)
{

You don't need the lock statement here.


Mattias

 

[Guest]

Thanks, Mattias!

I had earlier created the string array inside the for loop and had this
issue. Now it is back inside the for loop and I took off the lock statement.
It is behaving as I expected now.

private void ImportFiles()
{

//create a string array to pass to ImportFile
string[] importDetails = new string[2];

Here (unlike your first post) you're creating the array once outside
the foreach loop. By reusing the same array all the time you're likely
to see the behavior you initially described. Moving the array inside
the foreach loop should fix it.

currentFolderFiles = new DirectoryInfo(sourceFolderPath)).GetFiles();

You still have unbalanced parentheses here.

//make sure that we don’t exceed the number of threads specified
while (numberOfRunningThreads > (maxNumberThreads - 1))
{
//wait
Thread.Sleep(500);
}

This will never run since the local numberOfRunningThreads variable is
never incremented, only the _numberOfRunningThreads field.

lock(this)
{

You don't need the lock statement here.


Mattias

 

[Brian Gideon]

You need to synchronize reads of the numberOfRunningThreads variable as
well. Otherwise, changes made by other threads will not be seen and
you could end up with an infinite loop. You can use the
Interlocked.CompareExchange or Interlocked.Read methods to do that.

Better yet, use a semaphore to throttle the number of threads allowed
to enter a specified region of code. It's a cleaner solution than
incrementing/decrementing the numberOfRunningThreads variable and
attempting to block depending on its value.

Also, why limit the number of concurrently running work items in the
first place? Just queue them all up and let the ThreadPool do what it
does best. Don't worry about how many threads are executing.

Brian

Thanks for responding, Mattias!

Sorry about the code. Let me know if you have any issues with this:

//you will need the following references
using System.IO;
using System.Threading;

//add the following methods to a class, calling ImportFiles
private void ImportFiles()
{

//create a string array to pass to ImportFile
string[] importDetails = new string[2];

int numberOfRunningThreads = 0;
int maxNumberThreads = 4;

string sourceFolderPath = "E:\\Data\\TestUploadDocs";
string destinationPath = "E:\\Data\\Destination";

//navigate through all of the files in this folder
FileInfo[] currentFolderFiles;
currentFolderFiles = new DirectoryInfo(sourceFolderPath)).GetFiles();


foreach (FileInfo currentFile in currentFolderFiles)
{

//make sure that we don't exceed the number of threads specified
while (numberOfRunningThreads > (maxNumberThreads - 1))
{
//wait
Thread.Sleep(500);
}

//set the array to the file name and the doc library path
importDetails[0] = currentFile.FullName;
importDetails[1] = destinationPath;

//assign the method to the callback to the ImportOneFile method
WaitCallback callBack = new WaitCallback(ImportOneFile);

//interlock increment the number of threads running
Interlocked.Increment(ref _numberOfRunningThreads);

//create a thread on the queue and provide the method with the
string array
ThreadPool.QueueUserWorkItem(callBack, (object)importDetails);
}

//make sure all of the threads are completed processing before leaving method
int loopCount = 0;
while ((_numberOfRunningThreads > 0) && (loopCount < 6000))
{
Thread.Sleep(50);
loopCount++;
}

if (loopCount > 5999)
{
throw new Exception("Timeout waiting for threads to complete.");
}


}

private void ImportOneFile(object fileDetails)
{
//implement thread locking for protecting while using multithreading
lock(this)
{
//Using QueueUserWorkItem, parameters have to be sent to an object
// so here the parameters are repackaged in a string
string[] detailStrings = (string[])fileDetails;


Console.WriteLine("Inside ImportOneFile the file is {0} and the
destination path is {1}", detailStrings[0], detailStrings[1]);
}
//interlock decrement since we are done
Interlocked.Decrement(ref _numberOfRunningThreads);

}

Please post relevant parts of your actual code that compiles. The code
you posted have a number of errors in it, and it's pointless to guess
what's wrong if we're not looking at your actual code.



By using lock(this) (which is a bad thing in itself) around the entire
ImportOneFile method body you're effectively serializing access to
that code - only one thread at a time will run it. So the point of
having multiple threads here is sort of lost.


Mattias