Understanding class level variable usage in Multi-Threading C#

Question

I want to know the consequence of using a class level variable across different functions in a multi-threaded app.

I am creating a class variable and sharing it across get and set function. The variable is bound to have a value like :

"testuser-2.3" {username-projectversion}

Code:

class Test()
{
    private string key;

public Get(string something)
{
    key = setToSOmething();
}

public Set(string something)
{
    key = setToSOmething();
}

}

Is this code prone to fail in multithreaded environments? Like if two users are accessing diffrent versions of the project, will the "key" value will be diffrent at any random given point? Thanks in advance.

Answer 1

I want to know the consequence of using a class level variable across different functions in a multi-threaded app.

What you're doing in your code will work, sort of, but it doesn't demonstrate the consequence of allowing multiple threads to modify a variable. It doesn't answer your question. It just means that you'll be okay with the particular thing you're doing with this particular variable.

In this case you're just assigning a different reference to your string variable. That's safe, in a way. You won't get any mangled strings, but it means that you don't know which string a given function will get when it reads the variable. In some scenarios that's not so bad, but it's a little chaotic.

The problem occurs when multiple threads interact with your variable in a way that isn't thread safe.

Here's a really simple test method I wrote without actually knowing what was going to happen:

public class MultithreadedStringTest
{
    private string _sharedString;
    [TestMethod]
    public void DoesntMessUpStrings()
    {
        var inputStrings = "The quick fox jumped over the lazy brown dog".Split(' ');
        var random= new Random();
        Parallel.ForEach(Enumerable.Range(0, 1000), x =>
        {
            _sharedString += " " + inputStrings[random.Next(0, 9)];
        });
        var outputStrings = _sharedString.Trim().Split(' ');
        var nonMangledStrings = outputStrings.Count(s => inputStrings.Contains(s));
        Assert.AreEqual(1000, outputStrings.Length, 
            $"We lost {1000-outputStrings.Length} strings!");
        Assert.AreEqual(nonMangledStrings, outputStrings.Length, 
            $"{outputStrings.Length-nonMangledStrings} strings got mangled.");
    }
}

I'm starting with 10 words, and then, in a Parallel.Each loop appending 1000 words selected from those 10 to a single string from concurrent threads.

I expected the string to get mangled. It didn't. Instead what happened is that out of my 1000 words that I added, typically a few hundred just got lost.

We lost 495 strings!

Obviously that's not the particular operation that you're performing. But what it shows is that when we perform concurrent operations, we need to know that we're either calling a thread safe method or we're using some mechanism to prevent conflicts. We want to know how our code will behave and not cross our fingers and hope for the best.

If we're not careful with it the results will be unpredictable and inconsistent. It might work when you test it and fail later, and when it does it will be difficult to diagnose because you won't be able to see it happen.

Answer 2

Leaving aside the fact that Get and Set are both setting key in your code...

This code won't be prone to failure because of the nature of string. It's an immutable class, and the data is basically constructed elsewhere, and then your key assignment happens, as a single, atomic operation. (It's a reference or basically a pointer assignment).

So...even if you were to have two setters, key will always reference a valid string. Depending on the use of this code, though, the order in which the assignments actually happen could be counterintuitive. Say your get actually returns the string...set(get() + "X") could eventually lose Xes if called multiple times from multiple threads. Because all the get calls could get the same old string and perform the string addition on that. But this is you assuming set AND get together are an atomic operation. The accepted answer here: reference assignment is atomic so why is Interlocked.Exchange(ref Object, Object) needed? explains this better than I'm doing.

The contrary example would be if you were to use StringBuilder and actually modify the data inside the class...that would not be thread-safe and would certainly require a lock.

Updating answer to explain my reasoning behind my argument that the OP's code is fundamentally thread-safe considered on its own. Consider the following code which attempts to add thread-safety:

  public partial class MainWindow : Window
  {
    private object emergencyLock = new object();
    private string _status;
    public string status
    {
      get
      {
        //make sure if an emergency is happening we get the best possible string.
        lock (emergencyLock)
        {
          return _status;
        }
      }
      set
      {
        //we could also lock here instead of in GetEmergencyString()..which would fix the get+set=atomic issue
        _status = value;
      }
    }
    private string GetEmergencyString()
    {
      //this function understands an emergency is happening
      lock (emergencyLock)
      {
        //Maybe I'm fetching this string from a database, or building it by hand
        //It takes a while...We'll simulate this here
        Thread.Sleep(1000);
        return "Oh crap, emergency!";
      }
    }
    private void Normal_Button_Click(object sender, RoutedEventArgs e)
    {
      status = "Nothing much going on.";
    }

    private void Emergency_Button_Click(object sender, RoutedEventArgs e)
    {
      //GetEmergencyString() is evaluated first..finally returns a string, 
      //and THEN the assignment occurs as a single operation
      status = GetEmergencyString();
    }
  }

I'll make the following points about this code:

It does prevent a status seeker from getting a "boring" status during an emergency. It also potentially forces the status seeker to wait a full second before getting that status...Effectively solving nothing, most likely.

Also consider that even single-threaded, there's a fundamental issue here. The fundamental issue is NOT thread safety (in my opinion). The fundamental issue is delay. Better solutions? Fixing the delay. Active notification of the new state..Events, pubsub, etc. A state machine. Even a volatile bool IsEmergency is much better than the "thread-safety" I've added. Any active, intelligent logic in the code. Maybe you don't want the emergency state to be overwritten by the normal state? Again...not a threading issue.