c# Method looping and control.

Question

Hi guys,

I have a method that I would like to run over and over again.I would like to be able to start and stop this process.

I have been using this pattern for some socket work and I am wondering what improvements I can make?

public delegate void VoidMethod();

public class MethodLooper
{
    private VoidMethod methodToLoop;
    private volatile bool doMethod;
    private readonly object locker = new object();
    private readonly Thread loopingThread;

    public void Start()
    {
        if (!doMethod)
        {
            doMethod = true;
            loopingThread.Start();
        }
    }

    public void Stop()
    {
        if (doMethod)
        {
            doMethod = false;
            loopingThread.Join();
        }
    }

    public void ChangeMethod(VoidMethod voidMethod)
    {

        if (voidMethod == null)
            throw new NullReferenceException("voidMethod can't be a null");

        Stop();
        lock (locker)
        {
            methodToLoop = voidMethod;
        }
    }

    public MethodLooper(VoidMethod voidMethod)
    {
        if (voidMethod == null)
            throw new NullReferenceException("voidMethod can't be a null");
        methodToLoop = voidMethod;
        loopingThread = new Thread(new ThreadStart(_MethodLoop));
    }

    private void _MethodLoop()
    {
        VoidMethod methodToLoopCopy;
        while (doMethod)
        {
            lock (methodToLoop)
            {
                methodToLoopCopy = methodToLoop;
            }
            methodToLoopCopy();
        }
    }
}

Answer 1

Well you should express the doMethodToLoop as volatile. As stated in the MSDN:

"The volatile keyword indicates that a field can be modified in the program by something such as the operating system, the hardware, or a concurrently executing thread."

I am in a hurry but you should check out this tutorial from switch on the code.

Answer 2

A safer version would be to do it like this:

private readonly object m_locker = new object(); // readonly so it can never be null
private readonly Thread m_workerThread; // readonly so must set in constructor,
    // and never can be null afterwards

private Action m_methodToRun;
private volatile bool m_keepGoing = true; // needs to be volatile or else you need to lock around accesses to it.

public Constructor()
{
  m_workerThread = new Thread(ThreadWorker);
}

public void SetMethod(Action action)
{
  lock(m_locker)
    m_methodToRun = action;
}

private void ThreadWorker()
{
  while(m_keepGoing)
  {
    // use a lock to take a local copy in case another thread sets m_methodToRun to null
    // while we are processing things
    Action methodLocal;
    lock(m_locker)
      methodLocal = m_methodToRun;

    methodLocal(); // call it
    // Note: Remember that the underlying method being pointed to must ALSO be
    // thread safe. Nothing you do here can make up for that if it is not.
  }
}

private void Stop()
{ 
  m_keepGoing = false; 
  m_workerThread.Join(); // BLOCK and wait for it to finish
}

private void Start()
{ 
  m_keepGoing = true;
  m_workerThread.Start();
}

See this other question for the finer points on volatile vs locking