Using CountdownEvent and ManualResetEvent to control threads in ThreadPool

Question

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?

MSDN CountdownEvent

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.

You are asking the wrong questions. You haven't yet checked if your code is actually faster when you use threads. It is not. It thus doesn't make sense to try to make this dangerous and troublesome code work. — Hans Passant, Mar 17 '13 at 00:16
@HansPassant If the file sizes I'm working with are > 1 GB in size would threading help? — jordanhill123, Mar 17 '13 at 00:19

score 3 · Accepted Answer · edited May 23 '17 at 11:57

3

some notes:

wasn't supposed to be like this?:

CountdownEvent countdown = new CountdownEvent(files.Count());

Is it safe? - NO - I simply don't like the idea with CountdownEvent, if ANY operation with ANY file fails you don't get signal and application hangs on countdown.Wait(), I prefer using TPL Tasks instead - and instead of countdown.Wait() and use Task.WaitAll(tasks)

never use direct "foreach variable" in threads (this thread explains why), so instead of:

foreach (var f in files)
{
    Task.Run(() =>
    {
         var whateveryDoWithIt = f.File; 
    }
}

do this:

foreach (var f in files)
{
    var ftemp = f;
    Task.Run(() =>
    {
         var whateveryDoWithIt = ftemp.File; 
    }
}

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

edited May 23 '17 at 11:57

Community

1
1

answered Mar 16 '13 at 23:55

Tomas Panik

4,337
2
22
31

Yes it should be `files.Count()` Let me test out the Tasks library and see what I get. – jordanhill123 Mar 17 '13 at 00:15
I'll be using the Tasks library in the future. I didn't know it had all the functionality it does. Also with regards to the comment of @HansPassant above, I benchmarked my current code and the parallelization with my current functionality gives no benefit. I'll implement Tasks if I add to the functionality as its much simpler than what I had written. – jordanhill123 Mar 17 '13 at 00:40

Using CountdownEvent and ManualResetEvent to control threads in ThreadPool

1 Answers1