Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

Recently, I encountered an anomaly in the manner by which a worker thread was terminated. It prompted me to do some searching and I realized that I was constructing thread code in a haphazard manner. In hopes of beating this subject to death, I'd like some opinions, please.

The issue is real and serious; the use of a Thread.Abort locked up the application and prevented further disposals and terminations of a very formidable (I won't say "dangerous," but...) industrial device. Luckily programmable logic stepped in, but any safety systems anomaly is of great concern.

Consider a simple thread example:

using System;
using System.Diagnostics;
using System.Threading;

namespace RapperThreads
{
    class Program
    {
        static void Main(string[] args)
        {
            Debug.Assert(!rapperEnd.WaitOne(0));
            Console.Write("We choose");
            rapperRefrain = new string[] { " go to the moon,", " and do the other things," };
            for (int index = 0; index < rapperThread.Length; index++)
            {
                rapperThread[index] = new Thread(JFKRapper);
                rapperThread[index].IsBackground = true;
                rapperThread[index].Name = rapperRefrain[index];
                rapperThread[index].Start((object)index);
            }
            for (int index = 0; index < 3; index++)
            {
                Console.Write(" in this decade to");
                Thread.Sleep(900);
            }
            rapperEnd.Set();
            Thread.Sleep(0);
            Console.WriteLine(" not because they are easy, but because they are hard...");
            lock (rapperEnd)
                foreach (Thread thread in rapperThread)
                    if (thread != null)
                        if (thread.IsAlive)
                        {
                            try { thread.Abort(); }
                            catch (Exception ex) { Console.WriteLine(ex.ToString()); }
                            try { if (!thread.Join(100)) throw new Exception(); }
                            catch (Exception ex) { Console.WriteLine(ex.ToString()); }
                        }
            rapperThread[0] = rapperThread[1] = null;   //just for orderliness
            Console.ReadKey();
        }

        private static void JFKRapper(object indexObject)
        {
            int index = (int)indexObject;
            try
            {
                while (!rapperEnd.WaitOne(0))
                {
                    // Create something (say, a TCP connection) that must be explicitly
                    // closed/disposed.
                    try
                    {
                        while (!rapperEnd.WaitOne(500 + 100 * index))
                            // Do the expected work, over and over until some anomaly (exception or
                            // termination) is detected.
                            Console.WriteLine(rapperRefrain[index]);
                    }
                    catch (Exception ex)
                    {
                        // Report the exception anomaly if and only if termination has not been
                        // requested.
                        if (!rapperEnd.WaitOne(0))
                            Console.WriteLine(ex.ToString());
                    }
                    finally { /* Tidy/dispose of created something. */ }
                }
            }
            catch (Exception ex)
            {
                // Report the exception anomaly if and only if termination has not been requested.
                if (!rapperEnd.WaitOne(0))
                    Console.WriteLine(ex.ToString());
            }
            finally
            {
                // Tidy.
                lock (rapperEnd)
                    rapperThread[index] = null;
            }
        }

        private static ManualResetEvent rapperEnd = new ManualResetEvent(false);

        private static string[] rapperRefrain = null;

        private static Thread[] rapperThread = new Thread[2];
    }
}

Program.Main starts two threads and then enters a working loop of its own. The result is a mix of President Kennedy's Rice Moon speech and "We chose to go to the moon") that looks like this:

We choose in this decade to go to the moon,
and do the other things,
in this decade to go to the moon,
and do the other things,
go to the moon,
in this decade to and do the other things,
go to the moon,
and do the other things,
go to the moon,
not because they are easy, but because they are hard...

JFKRapper starts, makes something that must be explicitly and properly disposed of (well, the comments mark where this would be; imagine it's a TCP session), uses that something until told to stop (but does not use it too "frantically"), disposes of the something (TcpClients must be Closed as their finalization may occur only after an extended interval) and then ends. If, by some chance, a catastrophe occurs, JFKWrapper tidies the something, recreates it and continues performing good works until told to stop.

This is all pretty much vanilla stuff. OK, here are the questions:

  1. See that Abort near the end of Program.Main? It's wrapped in a try/catch block not only to announce the inevitable exception but to report that the Abort had to be used. However, to our horror, we watched that call lock up the application because the (equivalent) to the rapperEnd.Set statement took too long to complete (many subscribers to an event chain). Any ideas as to what's going on? ...and, No, "ensure that your thread ends benignly" is not an acceptable answer: the Abort is the never-fail last resort.

  2. In most cases, is it even necessary to check IsAlive other than to ensure a benign environment for the Join?

  3. Is it a good or bad idea to set rapperThread[index] = null in the outer finally paragraph of the worker method?

  4. See the Thread.Sleep(0) just after rapperEnd.Set? Does that actually give the worker thread any cycles? It doesn't seem to.

  5. Note that, at the end of Program.Main, what might be Dispose code uses foreach (Thread thread in rapperThread) to produce a local copy of the thread reference. That's to stop the reference from being "yanked out from under the feet" of the method. Good idea or bad?

  6. Now for the lock... The event rapperEnd is used as a lock to make more orderly the tidying of the thread reference(s), to much the same purpose as the aforementioned use of a local copy of the thread reference. The competitors are the outer finally paragraph in JFKWrapper and the thread termination code at the end of Program.Main. Again, good or bad idea; I'd sure hate to see a deadly embrace at an inopportune time.

  7. Assuming that the lock is a good idea, must the lock object (in this case, the ManaualResetEvent) be disposed of explicitly (it isn't)?

  8. Suppose you were to replace the ManualResetEvent with a CancellationToken. Just what is it that CancellationTokens give you that ManualResetEvents don't?

share|improve this question

migrated from stackoverflow.com Jun 13 '14 at 14:39

This question came from our site for professional and enthusiast programmers.

    
Side note: Join inside lock looks absolutely wrong as it is not "quick operation" locks are normally used for. – Alexei Levenkov Jun 13 '14 at 1:28
    
One thing of note: the program should call Join before Abort. – Dan Lyons Jun 13 '14 at 23:43
    
Re: Join before Abort I ABSOLUTELY agree but marvel at the example on MSDN at msdn.microsoft.com/en-us/library/5b50fdsz%28v=vs.85%29.aspx which shows the Abort before the Join. Just an unfortunate juxtaposition of commands in an attempt to show show related but not the same? – user1601638 Jun 14 '14 at 14:32
    
The lock on the thread method's null assignment to the thread reference is OK as long as, in the competing code (the Main), the event Set to instigate termination is outside of the same lock. – user1601638 Jun 14 '14 at 14:40

I think you definitely have a deadlock here with your use of rapperEnd. The main thread holds a lock on it while waiting for the worker threads to complete, and the worker threads need to acquire the lock in order to end. The problem is that the main thread holds the lock much longer than it needs to. In addition, rapperEnd has two totally unrelated responsibilities: as an Event and as a Monitor, which I would split into different objects.

Assuming that the lock is a good idea, must the lock object (in this case, the ManaualResetEvent) be disposed of explicitly (it isn't)?

You don't have to clean up objects you lock, but you should clean up events.

To address some other questions:

we watched that call lock up the application because the (equivalent) to the rapperEnd.Set statement took too long to complete (many subscribers to an event chain)

That's not how events work, setting an event should be nearly instantaneous. The Events classes on Windows are just thin wrappers around native Win32 event objects.

See the Thread.Sleep(0) just after rapperEnd.Set? Does that actually give the worker thread any cycles?

Sleep(0) tells the scheduler that the current thread doesn't need the remainder of its time slice. I wouldn't describe it as giving anything to another thread, it's all up to the scheduler to give processor time to threads.

share|improve this answer
    
Note that Sleep(0) has different behaviour to Sleep(1). Sleep(1) will suspend for 1 millisecond; Sleep(0) will not suspend if there are no other equal-priority threads to be executed. This is an important difference because it could essentially end up in the "sleeping" thread chewing up an entire core if nothing else is there for it to do. Sleep(1) is likely a better option here. stackoverflow.com/a/3257751/1073868 – Dan Pantry Mar 10 '15 at 16:45

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.