I've got the following multithreaded code excerpt that I've been working on to compare files following a zipped copy and unzip.
The application is zipping a folder containing a variable number of files of various sizes, copying the files to a server, and unzipping them. Then the files are compared and this comparison is threaded out to a ThreadPool
.
Here is the current full method:
public void FolderMoverLogic(string folderPathToZip, string unzipOutputDir)
{
string folderRootDir = Path.GetDirectoryName(folderPathToZip);
string folderNameToZip = Path.GetFileName(folderPathToZip);
try
{
//Zips files in <folderPathToZip> into folder <zippedLocal>
TransferMethods.CreateZipExternal(folderPathToZip, zippedlocal);
//Copies zipped folder to server location
File.Copy(zippedlocal + "\\" + folderNameToZip + ".zip", zippedserver + "\\" + folderNameToZip + ".zip");
//Unzips files to final server directory
TransferMethods.UnZip(zippedserver + "\\" + folderNameToZip + ".zip", unzipOutputDir + "\\" + folderNameToZip, sizeof(Int32));
TransferMethods m = new TransferMethods();
//Enumerate Files for MD5 Hash Comparison
var files = from file in Directory.EnumerateFiles(folderPathToZip, "*", SearchOption.AllDirectories)
select new
{
File = file,
};
int fileCount = 0;
CountdownEvent countdown = new CountdownEvent(10000);
using (ManualResetEvent resetEvent = new ManualResetEvent(false))
{
foreach (var f in files)
{
Interlocked.Increment(ref fileCount);
countdown.Reset(fileCount);
try
{
ThreadPool.QueueUserWorkItem(
new WaitCallback(c =>
{
//Check if any of the hashes have been different and stop all threads for a reattempt
if (m.isFolderDifferent)
{
resetEvent.Set();
CancellationTokenSource cts = new CancellationTokenSource();
cts.Cancel(); // cancels the CancellationTokenSource
try
{
countdown.Wait(cts.Token);
}
catch (OperationCanceledException)
{
Console.WriteLine("cde.Wait(preCanceledToken) threw OCE, as expected");
}
return;
}
else
{
//Sets m.isFolderDifferent to true if any files fail MD5 comparison
m.CompareFiles(f.File, folderRootDir, unzipOutputDir);
}
if (Interlocked.Decrement(ref fileCount) == 0)
{
resetEvent.Set();
}
countdown.Signal();
}));
}
catch (Exception ex)
{
Console.WriteLine(ex.ToString());
}
}
countdown.Wait();
resetEvent.WaitOne();
resetEvent.Close();
}
}
catch (Exception Ex)
{
Console.WriteLine(Ex.Message);
}
}
Useful resources looked at so far:
Is it safe to signal and immediately close a ManualResetEvent?
Stopping all thread in .NET ThreadPool?
ThreadPool Logic Requirements:
- Compare all enumerated files locally and on the server
- Return from all threads if hashing does not match
Previous ThreadPool Code:
using (ManualResetEvent resetEvent = new ManualResetEvent(false))
{
foreach (var f in files)
{
testCount++;
try
{
//Thread t = new Thread(() => m.CompareFiles(f.File, unzipped, orglsource));
//t.Start();
//localThreads.Add(t);
ThreadPool.QueueUserWorkItem(
new WaitCallback(c =>
{
if (resetEvent.WaitOne(0)) //Here is the `ObjectDisposedException`
{
return;
}
if (!m.Folderdifferent)
{
m.CompareFiles(f.File, folderRootDir, unzipOutput);
}
else
{
resetEvent.Set();
}
if (Interlocked.Decrement(ref fileCountZipped) == 0)
{
resetEvent.Set();
}
}));
}
catch (Exception ex)
{
Console.WriteLine(ex.ToString());
}
}
resetEvent.WaitOne();
}
I was getting ObjectDisposedExceptions
periodically with the previous code shown.
My questions are as such:
- Is the current method thread-safe?
- Is the logic sound?
- Any improvement ideas for performance or thread safety
- Does the current method I have at the top resolves the previous code exceptions
I've been testing this code and it has been working without exceptions but am looking at some more experienced feedback.
some notes:
countdown.Wait()
and useTask.WaitAll(tasks)
never use direct "foreach variable" in threads (this thread explains why), so instead of:
do this:to answer if it's thread-safe I would answer: Yes if you fix the points above and all methods used in it are also thread-safe