Thread safety in String class

Question

Is it thread safe to build strings from local variables using the String class like in the methods below? Suppose that the methods below are called from several threads.

public static string WriteResult(int value, string name)
{
    return string.Format("Result: value={0} name={1}", value, name);
}

public static string WriteResult2(int value, string name)
{
    return "Result: value=" + value + " name=" + name;
}

Or do I need to use StringBuilder to ensure thread safety?

score 20 · Accepted Answer · answered May 08 '15 at 09:47

That's absolutely fine. There is no shared state in either piece of code other than the string literals. As strings are immutable, it's fine for strings to be shared freely between threads, and both string.Format and string.Concat (implicitly called in the second piece of code) are thread-safe.

Even if one of the parameters were mutable and even if the method mutated the parameters, e.g.

public static void AddResult(int value, List<string> results)
{
    results.Add("Value " + value);
}

... then the method itself would still be thread-safe, so long as multiple threads didn't refer to the same List<string>. If multiple threads did refer to the same List<string> then it would be unsafe even if it just read from the list, as another thread could be mutating it.

score 18 · Answer 2 · edited May 23 '17 at 11:54

Both int and string as parameters in this method are effectively immutable and cannot be changed by outside code.

So there is no need to care about thread-safety with either Format method or String.Concat in this case.

But let us assume that we have some class MyObject that is mutable and can be changed from outside:

public class MyClass
{
    public Int32 value1 { get; set; }
    public String value2 { get; set;}
} 

public static string WriteResult2(MyObject obj)
{
    return "Result: value=" + obj.value1 + " name=" + obj.value2 ;
}

In this case whether with first approach or second you can return inconsistent value (it both value1 and value2 are changed after one value is already put to output.)

As @JonSkeet pointed it is not actually the method itself that is unsafe, but the class itself is unsafe to be shared in such manner between different threads.

To handle this situation you will have to either create special thread-safe instance method:

public class MyClass
{
    private Object lockObj = new Object();
    public Int32 value1
    {
        get
        {
            lock (this.lockObj) { ... });
        }
        set
        {
            lock (this.lockObj) { ... });
        }
    }
    public String value2
    {
        get
        {
            lock (this.lockObj) { ... });
        }
        set
        {
            lock (this.lockObj) { ... });
        }
    }

    public string WriteResult2()
    {
        lock (this.lockObj)
        {
             return "Result: value=" + this.value1 + " name=" + this.value2 ;
        }
    }
}

Or use some additional locking over such instances in methods that use it. The first in-class approach is obviously less error-prone, but may reduce performance and create a lot of boiler-plate code. Ideally, in concurrent programming the less you need to care about shared mutable state and its consistency the better.

I wouldn't say that your latter case makes the *method* thread-unsafe - it makes the *type* (MyClass) unsafe to share instances between multiple threads, which is a slightly different matter IMO. — Jon Skeet, May 08 '15 at 09:51
Your "thread-safe" version isn't actually thread-safe... because any other thread can modify `value1` and `value2` without acquiring a lock at all. — Jon Skeet, May 08 '15 at 10:15
A simpler solution would be to just state in documentation that *instance* members aren't thread-safe, as is typical. Very few types really need to be shared between threads - as I say in my answer, that doesn't mean that methods *using* those types as parameters aren't thread-safe, so long as the values passed to the method aren't shared... and that's a matter for the calling code. — Jon Skeet, May 08 '15 at 10:21
Do we really require a lock inside method `WriteResult2` although we are having locks inside property getter and setters? — Jenish Rabadiya, May 08 '15 at 10:22
@JenishRabadiya We need to ensure that values `value1` and `value2` must be consistent inside the `WriteResult` method - values cannot be changed while we are building the output string. For example `value1`=Jenish, `value2`=Rabadiya, we call `WriteResult`, it writes Jenish, but outer code at the same moment changes `value2` to Podskal. It is obviously not what we want. Still it allows us to make such inconsistent state before `WriteResult` call, so we should probably remove both setters and make one sync setter method for both properties. Or use immutable types. — Eugene Podskal, May 08 '15 at 10:27

score 5 · Answer 3 · answered May 08 '15 at 09:49

Both methods are thread-safe, because it does not matter what you do inside WriteResult. As long as it does not use mutable static state, and its parameters cannot be changed from the outside, your static method is thread-safe.

It is easy to verify that the methods do not use static state. It is also easy to verify that method's parameters cannot be changed from outside:

value cannot be changed, because it is of primitive type, and it is passed by value
name cannot be changed, because string objects are immutable.

Thread safety in String class

3 Answers3

Linked