Polling loop - Always a bad decision?

I had a need to isolate some long-running blocking calls in a background thread of my app. I also needed to keep this thread running indefinitely, because COM would complain if some object I created in that thread were accessed from another thread. I then needed to control what these objects did from my main UI thread, in a guaranteed-FIFO manner.

The solution I came up with was kind of ActiveObject-reminiscent; a polling loop running as the DoWork handler of a BackgroundWorker, which accepted encapsulated Command objects representing the work to perform from a thread-safe ConcurrentQueue. Here's the vitals:

```
//basic implementation of the Command pattern; no serialization/persistence needed,
//just need to be able to encapsulate some work and defer it.
internal class Command
{
private Action action;

public Command(Action action)
{
this.action = action;
}

protected Command()
{
}

public virtual void Execute()
{
action();
}
}

//Generic variation accepts a single parameter
internal class Command:Command
{
private T param;

private Action action;

public Command(Action action, T param)
{
this.action = action;
this.param = param;
}

public override void Execute()
{
action(param);
}
}

...

//event handlers run in the UI thread will Enqueue() Command objects
//containing the delegates that should be run in the background thread
ConcurrentQueue queuedCommands = new ConcurrentQueue();

//The BGW DoWork handler; runs indefinitely until the UI that needs it is Dispose()d
private void MainBackgroundLoop(object sender, System.ComponentModel.DoWorkEventArgs e)
{
Command command;
bool commandAvailable;

do
{
commandAvailable = queuedCommands.TryDequeue(out command);
if (commandAvailable)
command.Execute();
else
Thread.Sleep(100); //<-- Here's your code smell

You're right, most of the time, you shouldn't use polling like this. You should instead block the thread indefinitely if there is no work and wake it up when it's needed.

Fortunately, the same namespace that contains the ConcurrentQueue you use also has the solution you need: BlockingCollection. The collection has methods that will always return an item, and block if none are available, which is exactly what you need. Using that class, I would rewrite your code like this:

BlockingCollection queuedCommands = new BlockingCollection();

private void MainBackgroundLoop(object sender, DoWorkEventArgs e)
{
    do
    {
        Command command;
        bool commandAvailable = queuedCommands.TryTake(out command, Timeout.Infinite);
        if (commandAvailable)
            command.Execute();
    } while (commandAvailable);
}

Because the collection doesn't know about your DoWorkEventArgs, you have take care of ending the loop in some other way. In the code above, if you call queuedCommands.CompleteAdding(), it will make sure remaining items are processed and then it will shut down, because TryTake() will return false. Another option for cancellation is to use an overload of TryTake() that takes a CancellationToken and use that to cancel the operation.

StackExchange Code Review Q#9812, answer score: 15