Last time, I showed how compiler optimization can cause problems with polling for cancellation, and discussed a few alternatives to solving the problem. At the end of the article I mentioned that my simple program is the uncommon case. Very often (perhaps most often?), your program’s background processing is done by a method in a completely different class, and it’s common for there to be multiple background tasks running. It’s likely that you want a single method call or variable setting to signal cancellation for all of the background tasks.
Before I get to that, I want to address a few comments I received on Facebook in response to the last post.
Ron asks why this isn’t considered a compiler bug. On the face of it, it does look like the compiler is being overly aggressive. However, the compiler doesn’t know that the loop is executing in a separate thread. The compiler doesn’t understand that ThreadPool.QueueUserWorkItem starts a new background thread and that it should therefore widen its scope when examining the cancelRequested variable. Expecting the compiler to understand that is unreasonable, and forcing the compiler to assume that a local variable is accessible by multiple threads would eliminate a large number of perfectly valid optimizations. The compiler is behaving correctly here.
Readers Eric and Pete advocate moving the variable to class scope and adding the volatile qualifier. I agree that that’s the easiest solution and that it would work. I’d probably even use it if I were writing a simple program like the one in the last post. But I consider it a hack. Whereas Pete is right that an anonymous function closing over local variables muddies the concept of local variables, promoting the variable to class scope completely destroys any concept of information hiding. More importantly, there are the problems with volatile that I pointed out in the article. Those problems are irrelevant in this simple case, but I wouldn’t want to deal with them in a production program.
More importantly, as you’ll see below, volatile isn’t a good general solution.
Let’s move on from that toy demonstration program to a more likely real world design.
In the program below, the code that’s executed in the background thread is in a separate class that has no knowledge of or access to the code that calls it. It can’t access a public cancelRequested flag, so the flag is passed by reference.
public class Program
{
private static void Main(string[] args)
{
var p = new Program();
p.DoStuff();
Console.WriteLine("Press Enter to end program.");
Console.ReadLine();
}
private void DoStuff()
{
bool cancelRequested = false;
// Start tasks
ThreadPool.QueueUserWorkItem((s) =>
{
var task1 = new Task1Class();
task1.DoTask1(ref cancelRequested);
});
Console.WriteLine("Press Enter to stop thread.");
Console.ReadLine();
// set cancel flag
cancelRequested = true;
Console.WriteLine("Cancel requested!");
}
}
public class Task1Class
{
public void DoTask1(ref bool cancelRequested)
{
Console.WriteLine("Task1 started.");
while (!cancelRequested)
{
// do stuff
}
Console.WriteLine("Task1 exit.");
}
}If you run that in the debugger, it will work as expected. But run it without the debugger attached and the thread never exits! Once again, the compiler has optimized things.
Moving the cancelRequested flag to the outer scope doesn’t help, nor does moving it to the outer scope and marking it as volatile. In fact, adding volatile results in this compiler warning:
“a reference to a volatile field will not be treated as volatile.”
As with the example in my previous post, doing certain things in the loop will prevent the compiler from applying the optimization that reveals this programming bug. But do you really want to count on knowing what those rules are? For every platform your code will be running on, in all versions past, present, and future? I didn’t think so.
Passing the cancel flag by reference is a latent bug. Don’t do it.
I have to admit here that I gave some bad advice to my friend who first asked me about this. I was convinced that the compiler wouldn’t perform that optimization on a reference parameter. I was wrong, as this little example shows. I still don’t fully understand the optimization rules, but I’ve come to the conclusion that the compiler assumes single-threaded code in most situations. That makes sense, actually. The compiler would have to avoid a whole host of optimizations if it couldn’t assume single-threaded code.
If you’re writing multi-threaded code, you need to use synchronization primitives in order to control things. Prior to .NET 4.0, a common technique was to use a ManualResetEvent, like this:
private void DoStuff()
{
ManualResetEvent cancelRequested = new ManualResetEvent(false); // <----
// Start tasks
ThreadPool.QueueUserWorkItem((s) =>
{
var task1 = new Task1Class();
task1.DoTask1(cancelRequested); // <----
});
Console.WriteLine("Press Enter to stop thread.");
Console.ReadLine();
// set cancel flag
cancelRequested.Set();
Console.WriteLine("Cancel requested!");
}
public class Task1Class
{
public void DoTask1(ManualResetEvent cancelRequested) // <----
{
Console.WriteLine("Task1 started.");
while (!cancelRequested.WaitOne(0)) // <----
{
// do stuff
}
Console.WriteLine("Task1 exit.");
}
}The lines with “// <----” comments are the only ones that change.
You can still use that technique in .NET 4.0 or later. You can get slightly better performance using ManualResetEventSlim. I see only two problems with using the events this way.
First, the expression !cancelRequested.WaitOne(0) isn’t exactly clear. It’s not as bad as the Interlocked.CompareExchange example from my last post, but it’s definitely not as clear as !cancelRequested.
More importantly, any thread can call cancelRequested.Set to cancel the entire operation. Granted, background tasks shouldn’t do that, but they could. It would be better if we could pass a read-only token of some sort.
.NET 4.0 formalized the concept of Cancellation, implemented with the CancellationTokenSource and CancellationToken classes. Using these classes, you can implement cancellation in a standard, safe, and reliable manner.
The basic idea is that the main program creates a CancellationTokenSource instance, and passes the Token member (a CancellationToken) to the methods that want to check for cancellation. I’ll let you read up on the details.
Here’s an example.
private void DoStuff()
{
CancellationTokenSource cancel = new CancellationTokenSource();
// Start tasks
ThreadPool.QueueUserWorkItem((s) =>
{
var task1 = new Task1Class();
task1.DoTask1(cancel.Token);
});
Console.WriteLine("Press Enter to stop thread.");
Console.ReadLine();
// set cancel flag
cancel.Cancel();
Console.WriteLine("Cancel requested!");
}
public class Task1Class
{
public void DoTask1(CancellationToken cancelToken)
{
Console.WriteLine("Task1 started.");
while (!cancelToken.IsCancellationRequested)
{
// do stuff
}
Console.WriteLine("Task1 exit.");
}
}That looks a lot cleaner to me. I can tell just by looking at the code exactly what is going on. Furthermore, there is no way that one of the tasks can inadvertently cancel the entire process because there’s no way for them to set the cancel flag. They can only query it.
Another advantage of this method (and the one that uses events) over the reference flag (which doesn’t work, but even if it did) is that you can pass the CancellationToken to a class’s constructor so that the class can cache it. That way, it doesn’t have to pass the cancellation token to every internal method that might need it. For example:
private void DoStuff()
{
CancellationTokenSource cancel = new CancellationTokenSource();
// Start tasks
ThreadPool.QueueUserWorkItem((s) =>
{
var task1 = new Task1Class(cancel.Token);
task1.DoTask1();
});
Console.WriteLine("Press Enter to stop thread.");
Console.ReadLine();
// set cancel flag
cancel.Cancel();
Console.WriteLine("Cancel requested!");
}
public class Task1Class
{
private readonly CancellationToken _cancelToken;
public Task1Class(CancellationToken cancelToken)
{
_cancelToken = cancelToken;
}
public void DoTask1()
{
Console.WriteLine("Task1 started.");
DoSubtask1();
if (!_cancelToken.IsCancellationRequested)
{
DoSubtask2();
}
Console.WriteLine("Task1 exit.");
}
private void DoSubtask1()
{
while (!_cancelToken.IsCancellationRequested)
{
// do stuff
}
}
private void DoSubtask2()
{
while (!_cancelToken.IsCancellationRequested)
{
// do stuff
}
}
}You can’t store a reference in C# (you can store an instance of a reference type, but that’s a different thing), so there wouldn’t be any way to do this using a simple Boolean flag. You can’t write, for example:
private ref bool cancelRequested; // this doesn't compile!The most important thing I learned from this investigation is:
If you’re writing multi-threaded code, use synchronization primitives for all inter-thread communication.
Furthermore, I’ve shown that volatile is not a synchronization primitive.
If you’re writing for .NET 4.0 or later, then take advantage of Cancellation. Otherwise, use a ManualResetEvent or ManualResetEventSlim to approximate that functionality.