views:

126

answers:

4

Problem statement

I have a worker thread that basically scans a folder, going into the files within it, and then sleeps for a while. The scanning operation might take 2-3 seconds but not much more. I'm looking for a way to stop this thread elegantly.

Clarification: I want to stop the thread while it's sleeping, and not while it's scanning. However, the problem is that I do not know what is the current state of the thread. If it's sleeping I want it to exit immediately. If it's scanning, I want it to exit the moment it tries to block.

Attempts at a solution

At first I was using Sleep and Interrupt. Then I found out that Interrupt doesn't really interrupt the Sleep - it only works when the threads TRIES to go into sleeping.

So I switched to Monitor Wait&Pulse. Then I found out that the Pulse only works when I'm actually in the Wait. So now I have a thread which looks like that:

while (m_shouldRun)
{
    try
    {
        DoSomethingThatTakesSeveralSeconds();
        lock (this)
        {
            Monitor.Wait(this, 5000);
        }
    }
    catch (ThreadInterruptedException)
    {
        m_shouldRun = false;
    }
}

And now I need to craft my Stop function. So I started with:

public void Stop()
{
    m_shouldRun = false;
    lock (this)
    {
        Monitor.Pulse(this);
    }
    thread.Join();
}

But this doesn't work because I may be pulsing while the thread works (while it's not waiting). So I added Interrupt:

public void Stop()
{
    m_shouldRun = false;
    thread.Interrupt();
    lock (this)
    {
        Monitor.Pulse(this);
    }
    thread.Join();
}

Another option is to use:

public void Stop()
{
    m_shouldRun = false;
    while (!thread.Join(1000))
    {
        lock (this)
        {
            Monitor.Pulse(this);
        }
    }
}

The question

What is the preferred method? Is there a third method which is preferable?

+7  A: 

The way to stop a thread elegantly is to leave it finish by itself. So inside the worker method you could have a boolean variable which will check whether we want to interrupt. By default it will be set to false and when you set it to true from the main thread it will simply stop the scanning operation by breaking from the processing loop.

Darin Dimitrov
+1 for allowing thread to finish by itself. Any other approach is messy. Don't forget to mark the boolean flag with the volatile keyword.
spender
Thanks. You'll notice that I do have such a flag. I do not interrupt the thread while it's actually working, but I do want to interrupt it while it's sleeping. If it's going to sleep 10 minutes, I don't want it to continue sleeping.
Eldad Mor
A thread that's sleeping for 10 seconds is not useful to anyone. Use the `ThreadPool` to draw threads whenever you need to perform some tasks but don't leave them sleeping. Make them do useful stuff.
Darin Dimitrov
@Darin: This thread should periodically scan a folder. It's not event-based. How would you suggest I take an instance from the ThreadPool and run the scan every 10 minutes? Also, it's not like I have a resources problem here, because I don't have many threads such as this one in my application. I guess I could be using timers, but I'm not sure I like that idea.
Eldad Mor
*How would you suggest I take an instance from the ThreadPool and run the scan every 10 minutes*: By using the [ThreadPool.RegisterWaitForSingleObject](http://msdn.microsoft.com/en-us/library/system.threading.threadpool.registerwaitforsingleobject.aspx) method and setting the `executeOnlyOnce` parameter to false. When you decode to stop simply unregister the wait handle with [RegisteredWaitHandle.Unregister](http://msdn.microsoft.com/en-us/library/system.threading.registeredwaithandle.unregister.aspx).
Darin Dimitrov
Interesting. Let's see if I got it right. From my main thread I call RegisterWaitForSingleObject, with, say, 10min timeout, and with the "DoSomething" function as the callBack. I never signal the waitObject, so the trigger will only occur after 10 minutes. Once the callback is done - the wait will start again. Isn't that just delegating the "thread" to .NET? I mean - the waiting thread will exist anyhow, won't it?
Eldad Mor
@Eldad Mor, alternative to sleeping 10 minutes is splitting that time to sleep for instance 10 consequent times by 1 minute and checking whether exit flag was set.
Regent
@Regent, wouldn't that be a busy wait (even though it will be sleeping most of the time)? If I want the thread to exit "immediately" I'll need to divide the interval into 10ms sections under that method.
Eldad Mor
A: 

I recommend to keep it simple:

while (m_shouldRun)
{
    DoSomethingThatTakesSeveralSeconds();
    for (int i = 0; i < 5; i++)  // example: 5 seconds sleep
    {
        if (!m_shouldRun)
            break;
        Thread.Sleep(1000);
    }
}

public void Stop()
{
    m_shouldRun = false;
    // maybe thread.Join();
}

This has the following advantages:

  • It smells like busy waiting, but it's not. $NUMBER_OF_SECONDS checks are done during the waiting phase, which is not comparable to the thousands of checks done in real busy waiting.
  • It's simple, which greatly reduces the risk of error in multi-threaded code. All your Stop method needs to do is to set m_shouldRun to false and (maybe) call Thread.Join (if it is necessary for the thread to finish before Stop is left). No synchronization primitives are needed (except for marking m_shouldRun as volatile).
Heinzi
The DoSomething function will not be forcefully interrupted. Thread.Interrupt only "happens" when the thread tries to block. See the MS documentation (here: http://msdn.microsoft.com/en-us/library/system.threading.thread.interrupt.aspx) - "If this thread is not currently blocked in a wait, sleep, or join state, it will be interrupted when it next begins to block."
Eldad Mor
@Eldad: Good point, I confused it with Thread.Abort. Changed my answer.
Heinzi
+4  A: 

Another alternative is to use events:

private ManualResetEvent _event = new ManualResetEvent(false);


public void Run() 
{
 while (true)
 {
    DoSomethingThatTakesSeveralSeconds();
    if (_event.WaitOne(timeout))
      break;
 }
}

public void Stop() 
{
   _event.Set();
   thread.Join();
}
liggett78
Yes, this will work as well. The question is what would be the best way of doing it. Your option does look better than the recurring Pulse or the Interrupt+Pulse.
Eldad Mor
+1, yes, this is *far* better.
Hans Passant
Well, personally I would not use Pulse/Interrupts. Pulse might be problematic due to possible APCs (I believe it might go unnoticed by the thread). Interrupts don't have a good reputation: http://www.bluebytesoftware.com/blog/2007/08/23/ThreadInterruptsAreAlmostAsEvilAsThreadAborts.aspx
liggett78
If you have only one thread doing the job, go with an event. Otherwise with a volatile flag and a timeout to wake-up and check whether the flag is set or not (avoids creating a lot of events which are kernel objects).
liggett78
A: 

I came up with separately scheduling the task:

using System;
using System.Threading;

namespace ProjectEuler
{
    class Program
    {
        //const double cycleIntervalMilliseconds = 10 * 60 * 1000;
        const double cycleIntervalMilliseconds = 5 * 1000;
        static readonly System.Timers.Timer scanTimer =
            new System.Timers.Timer(cycleIntervalMilliseconds);
        static bool scanningEnabled = true;
        static readonly ManualResetEvent scanFinished =
            new ManualResetEvent(true);

        static void Main(string[] args)
        {
            scanTimer.Elapsed +=
                new System.Timers.ElapsedEventHandler(scanTimer_Elapsed);
            scanTimer.Enabled = true;

            Console.ReadLine();
            scanningEnabled = false;
            scanFinished.WaitOne();
        }

        static void  scanTimer_Elapsed(object sender,
            System.Timers.ElapsedEventArgs e)
        {
            scanFinished.Reset();
            scanTimer.Enabled = false;

            if (scanningEnabled)
            {
                try
                {
                    Console.WriteLine("Processing");
                    Thread.Sleep(5000);
                    Console.WriteLine("Finished");
                }
                finally
                {
                    scanTimer.Enabled = scanningEnabled;
                    scanFinished.Set();
                }
            }
        }
    }
}
Regent