Is it ok to use Thread.Sleep and Thread.Interrupt for pausing and resuming Thread like this?

Question

I need to observe a ConcurrentQueue, but to minimize the resources I want to pause the Thread if the Queue is empty and resume it from another Thread if there is a new Entry in the Queue. I implemented a pausing and an resuming of a thread like this:

My Worker, where the DoWork() method is called in a new Thread, looks like this:

public static class Worker
{
    public static bool Running = true;

    public static void DoWork()
    {
        while (Running)
        {
            try
            {
                Thread.Sleep(Timeout.Infinite);
            }
            catch (ThreadInterruptedException)
            {
                DoActualWork();
            }
        }
    }

    private static void DoActualWork()
    {
        //Do something
    }
}

I start the thread like this:

Thread workerThread = new Thread(Worker.DoWork);
workerThread.Start();

I interrupt the Thread like this:

workerThread.Interrupt();

I stop the Thread like this:

Worker.Running = false;

Everything is working as expected but I'm not sure if this is how it should be implemented.

1. Is this best practice?
2. What can go wrong?
3. Is there a problem with the static class and the members? (It have to be static because the Thread have to be interrupted by different Threads)

Answer 1

This is pretty bad.

Instead why not use BlockingCollection. Then the loop will be:

try
{
    while(true)
    {
        doSomethingWith(queue.Take());
    }
}
catch(InvalidOperationException e)
{
    // ignore and cleanup
}

And to stop the thread you need to call CompleteAdding() on the queue.

Answer 2

• Is this best practice?

  No. Assuming there wouldn't be any call of the workerThread.Interrupt() method, this DoWork() method can't be stopped by just setting Worker.Running = false; because at this point the current thread is still sleeping.

• What can go wrong?

  The compiler can optimize your code in a way that the Running variable won't reflect the actual value, because the compiler assumes the access only to be from a single thread. Changing the declaration to public static volatile bool Running = true; will ensure that the most up-to-date value is present in the field at all times and no compiler optimizations will be done for this field. See: volatile

• Is there a problem with the static class and the members?

  See above

• How to handle this ?

  That what Timer's are for. If the Timer raises the Elapsed event, you process the items in the ConcurrentQueue until the queue is empty. Then you start the timer again and wait for the event to be raised again.

---

Answer 3

Your question is a perfect fit for Reactive Extensions. The concept is that there is something that you want to subscribe. At some point in the future something will happen. You don't care when you just want the data... This code lifted from Lee Campbell's excellent tutorial on the matter.

private IObservable<string> NonBlocking()
{
  return Observable.Create<string>(
 (IObserver<string> observer) =>
 {
   observer.OnNext("a");
   observer.OnNext("b");
   observer.OnCompleted();
   Thread.Sleep(1000);
   return Disposable.Create(() => Console.WriteLine("Observer has unsubscribed"));
    //or can return an Action like 
    //return () => Console.WriteLine("Observer has unsubscribed"); 
 });
}

In the method above "NonBlocking", it returns a type of IObservable... which has a method named Subscribe. Once this method is called the caller only has to call the subscribe method of the observable and can do anything it wants. When the string is ready, it is injected into the parameter of the subscriber on that thread it's running. So you don't have to fool around with thread either...

The caller code would look like this:

var observable = OtherClass.NonBlocking();
observable.Subscribe(p=>{  // the data is contained here } );