How do I know if this C# method is thread safe?

Question

I'm working on creating a call back function for an ASP.NET cache item removal event.

The documentation says I should call a method on an object or calls I know will exist (will be in scope), such as a static method, but it said I need to ensure the static is thread safe.

Part 1: What are some examples of things I could do to make it un-thread safe?

Part 2: Does this mean that if I have

static int addOne(int someNumber){
    int foo = someNumber;
    return foo +1; 
}

and I call Class.addOne(5); and Class.addOne(6); simutaneously, Might I get 6 or 7 returned depending on who which invocation sets foo first? (i.e. a race condition)

Solution 1

That addOne function is indeed thread safe because it doesn't access any data that could be accessed by another thread. Local variables cannot be shared among threads because each thread gets its own stack. You do have to make sure, however, that the function parameters are value types and not reference types.

static void MyFunction(int x) { ... } // thread safe. The int is copied onto the local stack.

static void MyFunction(Object o) { ... } // Not thread safe. Since o is a reference type, it might be shared among multiple threads. 

Solution 2

No, addOne is thread-safe here - it only uses local variables. Here's an example which wouldn't be thread-safe:

 class BadCounter
 {
       private static int counter;

       public static int Increment()
       {
             int temp = counter;
             temp++;
             counter = temp;
             return counter;
       }
 }

Here, two threads could both call Increment at the same time, and end up only incrementing once. (Using return ++counter; would be just as bad, by the way - the above is a more explicit version of the same thing. I expanded it so it would be more obviously wrong.)

The details of what is and isn't thread-safe can be quite tricky, but in general if you're not mutating any state (other than what was passed into you, anyway - bit of a grey area there) then it's usually okay.

Solution 3

Threading issues (which I've also been worrying about lately) arise from the use of multiple processor cores with separate caches, as well as from basic thread-swapping race conditions. If the caches for separate cores access the same memory location, they will generally have no idea about the other one and may separately track the state of that data location without it going back out to main memory (or even to a synchronized cache shared across all cores at L2 or L3, for example), for processor performance reasons. So even order-of-execution interlock tricks may not be reliable in multi-threaded environments.

As you may know, the main tool to correct for this is a lock, which provides a mechanism for exclusive access (between contentions for the same lock) and handles the underlying cache synchronization so that accesses to the same memory location by various lock-protected code sections will be properly serialized. You can still have race conditions between who gets the lock when and in what order, but that's usually much simpler to deal with when you can guarantee that execution of a locked section is atomic (within the context of that lock).

You can get a lock on an instance of any reference type (eg. inherits from Object, not value types like int or enums, and not null), but it's very important to understand that the lock on an object has no inherent effect on accesses to that object, it only interacts with other attempts to get the lock on the same object. It is up to the class to protect access to its member variables using an appropriate locking scheme. Sometimes instances might protect multi-threaded accesses to their own members by locking on themselves (eg. lock (this) { ... } ), but usually this is not necessary because instances tend to be held by only one owner and don't need to guarantee threadsafe access to the instance.

More commonly, a class creates a private lock (eg. private readonly object m_Lock = new Object(); for separate locks within each instance to protect access to members of that instance, or private static readonly object s_Lock = new Object(); for a central lock to protect access to the class's static members). Josh has a more specific code example of using a lock. You then have to code the class to use the lock appropriately. In more complex cases you might even want to create separate locks for different groups of members, to reduce contention for different kinds of resources which aren't used together.

So, to bring it back to your original question, a method which only accesses its own local variables and parameters would be thread-safe, because these exist in their own memory locations on the stack specific to the current thread, and can not be accessed elsewhere--unless you shared those parameter instances across threads before passing them.

A non-static method which only accesses the instances own members (no static members)--and of course parameters and local variables--would not need to use locks in the context of that instance being used by a single owner (doesn't need to be thread-safe), but if instances were intended to be shared and wanted to guarantee thread-safe access, then the instance would need to protect access to its member variables with one or more locks specific to that instance (locking on the instance itself being one option)--as opposed to leaving it up to the caller to implement their own locks around it when sharing something not intended to be thread-safe shareable.

Access to readonly members (static or non-static) which aren't ever manipulated is generally safe, but if the instance it holds is not itself thread-safe or if you need to guarantee atomicity across multiple manipulations of it, then you may need to protect all access to it with your own locking scheme as well. That's a case where it could be handy if the instance uses locking on itself, because you could simply get a lock on the instance across multiple accesses into it for atomicity, but you wouldn't need to do so for single accesses into it if it's using a lock on itself to make those accesses individually thread-safe. (If it's not your class, you'd have to know whether it locks on itself or is using a private lock which you can't access externally.)

And finally, there's access to changing static members (changed by the given method or by any others) from within an instance--and of course static methods which access those static members and could be called from anyone, anywhere, anytime--which have the biggest need to use responsible locking, without which are definitely not thread-safe and are likely to cause unpredictable bugs.

When dealing with .NET framework classes, Microsoft documents in MSDN whether a given API call is thread-safe (eg. static methods of the provided generic collection types like List<T> are made thread-safe while instance methods may not be--but check specifically to be sure). The vast majority of the time (and unless it specifically says it's thread-safe), it's not internally thread-safe, so it's your responsibility to use it in a safe manner. And even when individual operations are implemented internally thread-safe, you still have to worry about shared and overlapping access by your code if it does anything more complex which needs to be atomic.

One big caveat is iterating over a collection (eg. with foreach). Even if each access to the collection gets a stable state there's no inherent guarantee that it won't change in between those accesses (if anywhere else can get to it). When the collection is held locally there's generally no problem, but a collection which could be changed (by another thread or during your loop's execution!) could produce inconsistent results. One easy way to solve this is to use an atomic thread-safe operation (inside your protective locking scheme) to make a temporary copy of the collection (MyType[] mySnapshot = myCollection.ToArray();) and then iterate over that local snapshot copy outside the lock. In many cases this avoids the need for holding a lock the whole time, but depending on what you're doing within the iteration this may not be enough and you just have to protect against changes the whole time (or you may already have it inside a locked section guarding against access to change the collection along with other things, so it's covered).

So, there's a bit of an art to thread-safe design, and knowing just where and how to get locks to protect things depends a lot on the overall design and usage of your class(es). It can be easy to get paranoid and think you have to get locks all over for everything, but really it's about finding the right layer at which to protect things.

Solution 4

Your method is fine since it is only using local variables, let's change your method a bit:

static int foo;

static int addOne(int someNumber)
{
  foo=someNumber; 
  return foo++;
}

This is not a thread safe method because we are touching static data. This would then need to be modified to be:

static int foo;
static object addOneLocker=new object();
static int addOne(int someNumber)
{
  int myCalc;
  lock(addOneLocker)
  {
     foo=someNumber; 
     myCalc= foo++;
  }
  return myCalc;
}

Which I think this is a silly sample I just did cause if I'm reading it correctly there is no point in foo anymore but hey it's a sample.

Solution 5

There is some research going on which allows you to detect non-thread-safe code. E.g. the project CHESS at Microsoft Research.

View more solutions

Comments

[Jon Skeet]

Your first statement is incorrect. Two threads could both have a reference to a string, and could both call methods on the string at the same time with no ill effects. Even a mutable object can be thread-safe with care - even without locking, if you use Interlocked etc. It's not easy, but possible.

[JoshBerke]

If your passed value or an immuatable type like string you should be safe to mutate it correct? If your passed a reference object then that's where it become grey?

[Dave Costa]

@Josh: In general, if you're passing something to the method, then modifying that item is thread safe because each caller would pass its own item. The key in Jon's example is that each thread would be modifying the same static member "counter".

[Jon Skeet]

If it's immutable, you won't be able to mutate it :) Calling a method which just creates a new instance (like string.Replace does) is safe though. (A string is still a reference type, by the way.) The grey area is if you're passed a reference to a mutable object - instance methods on it (cont)

[Jon Skeet]

may not be safe, but you could regard that as the caller's problem to sort out (i.e. two threads can call the method at the same time, but with different arguments).

[Thomas Danecker]

MethodImplOptions.Synchronized is not the ideal choice because it locks on this. I'd prefer to lock on a separate, private member to hide these implementation details from the outer world (Separation of Concerns, ...)

[Jacobs Data Solutions]

I don't understand a word you just said but +1 for the effort.

[JJoos]

You should start a blog, then you wouldn't need the excuse of questions to write articles!

[Jeroen Landheer]

I don't think this will compile, because static members cannot access instance members. (addOne can't access myVar.)

[TheSmurf]

You're right it won't. Much too long in the past to remember why it's like that, so just corrected it.

[RBT]

What if x is a value type but is passed by reference - static void MyFunction(ref int x) { ... }. Will it still be thread-safe?

[RBT]

Your basic definition related to state mutation certainly gives peace of mind when someone is trying to understand thread-safety but this certainly doesn't end here and then it all suddenly becomes a whirlpool of complications.