CODEWITHSUNDEEP
c#asp.netmultithreading
fs2005
fs2005

Reputation: 591

Is this operation thread safe?

In the following example when the "Submit" button is clicked the value of the static variable Count is incremented. But is this operation thread safe? Is using Appliation object the proper way of doing such operation? The questions are applicable to Web form application s as well.

The count always seem to increase as I click the Submit button.

View(Razor):

@{
    Layout = null;
}
<html>

<body>
    <form>
        <p>@ViewBag.BeforeCount</p>
        <input type="submit" value="Submit" />
    </form>
</body>
</html>

Controller:

public class HomeController : Controller
{
    public ActionResult Index()
    {
        ViewBag.BeforeCount = StaticVariableTester.Count;
        StaticVariableTester.Count += 50;
        return View();
    }     
}

Static Class:

public class StaticVariableTester
{
    public static int Count;
}

Upvotes: 12

Views: 1248

Answers (3)

Alex Erygin
Alex Erygin

Reputation: 3230

If a method (instance or static) only references variables scoped within that method then it is thread safe because each thread has its own stack. You also can achieve thread safety by using any variety of synchronization mechanisms.

This operation is not thread safe because it uses shared variable: ViewBag.BeforeCount.

What Makes a Method Thread-safe? What are the rules?

Upvotes: 4

dcastro
dcastro

Reputation: 68660

No, it's not. The += operator is done in 3 steps: read the value of the variable, increase it by one, assign the new value. Expanded:

var count = StaticVariableTester.Count;
count = count + 50;
StaticVariableTester.Count = count;

A thread could be preempted between any two of these steps. This means that if Count is 0, and two threads execute += 50 concurrently, it's possible Count will be 50 instead of 100.

  1. T1 reads Count as 0.
  2. T2 reads Count as 0
  3. T1 adds 0 + 50
  4. T2 adds 0 + 50
  5. T1 assigns 50 to Count
  6. T2 assigns 50 to Count
  7. Count equals 50

Additionally, it could also be preempted between your first two instructions. Which means two concurrent threads might both set ViewBag.BeforeCount to 0, and only then increment StaticVariableTester.Count.

Use a lock

private readonly object _countLock = new object();

public ActionResult Index()
{
    lock(_countLock)
    {
        ViewBag.BeforeCount = StaticVariableTester.Count;
        StaticVariableTester.Count += 50;
    }
    return View();
}

Or use Interlocked.Add

public static class StaticVariableTester
{
    private static int _count;

    public static int Count
    {
        get { return _count; }
    }

    public static int IncrementCount(int value)
    {
        //increments and returns the old value of _count
        return Interlocked.Add(ref _count, value) - value;
    }
}

public ActionResult Index()
{
    ViewBag.BeforeCount = StaticVariableTester.IncrementCount(50);
    return View();
}

Upvotes: 13

dav_i
dav_i

Reputation: 28107

Increment is not atomic so is not thread safe.

Check out Interlocked.Add:

Adds two 32-bit integers and replaces the first integer with the sum, as an atomic operation.

You'd use it like this:

Interlocked.Add(ref StaticVariableTester.Count, 50);

Personally I'd wrap this in your StaticVariableTester class:

public class StaticVariableTester
{
    private static int count;

    public static void Add(int i)
    {
        Interlocked.Add(ref count, i);
    }

    public static int Count
    {
        get { return count; }
    }
}

If you want the returned values (as per dcastro's comment) then you could always do:

public static int AddAndGetNew(int i)
{
     return Interlocked.Add(ref count, i);
}

public static int AddAndGetOld(int i)
{
     return Interlocked.Add(ref count, i) - i;
}

In your code you could do

ViewBag.BeforeCount = StaticVariableTester.AddAndGetOld(50);

Upvotes: 5

Related Questions