Is this operation thread safe?

Question

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;
}

http://www.codeproject.com/Articles/37976/Writing-Thread-Safe-Code-in-C — , Apr 03 '14 at 11:52
No.Please read these questions: http://stackoverflow.com/questions/1254750/i-need-to-create-a-thread-safe-static-variable-in-c-sharp-net http://stackoverflow.com/questions/6941181/are-static-variables-thread-safe-c-sharp http://stackoverflow.com/questions/8907612/mspec-how-to-make-static-variables-thread-safe — , Apr 03 '14 at 11:54
Static variables to increment items are rarely a good way to do things in a web based situation. What are you trying to do. — Paddy, Apr 03 '14 at 11:56
`Static` and thread safety are orthogonal, please read about thread safety. — Sriram Sakthivel, Apr 03 '14 at 11:59
@Bharadwaj My favourites [this](http://blogs.msdn.com/b/ericlippert/archive/2009/10/19/what-is-this-thing-you-call-thread-safe.aspx) and [this](http://www.albahari.com/threading/part2.aspx#_Thread_Safety). They are not enough google will give you more :) — Sriram Sakthivel, Apr 03 '14 at 12:06
@Sriram Sakthivel Thanks :) "A programmer is always a learner". :) — Bharadwaj, Apr 03 '14 at 12:07

dcastro · Accepted Answer · 2014-04-03T12:42:38.123

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.

T1 reads Count as 0.
T2 reads Count as 0
T1 adds 0 + 50
T2 adds 0 + 50
T1 assigns 50 to Count
T2 assigns 50 to Count
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();
}

dav_i · Answer 2 · 2014-04-03T12:37:31.527

5

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);

edited Apr 03 '14 at 12:37

answered Apr 03 '14 at 11:53

dav_i

27,509
17
104
136

Probably want `Interlocked.Add` – ta.speot.is Apr 03 '14 at 11:53
What this does? He adds 50! not just `var++` – Sriram Sakthivel Apr 03 '14 at 11:54
2

In addition to not being thread-safe, it won't work across servers in the case of a web farm. – John Saunders Apr 03 '14 at 11:54
@ta.speot.is Yep. Updated. – dav_i Apr 03 '14 at 11:54
1

That won't work if `Count` is a property, which it should be. – dcastro Apr 03 '14 at 11:55
That's still not thread-safe, a thread could be preempted between reading `Count` into `ViewBag.BeforeCount` and incrementing `Count`. – dcastro Apr 03 '14 at 12:05
@dcastro The question as I understood it was whether the increment was thread safe, not the read as well. – dav_i Apr 03 '14 at 12:23
@dav_i I'm pretty sure the OP didn't realize that there's more to thread-safety here than the increment. – dcastro Apr 03 '14 at 12:39
2

@dcastro Yeah fair enough about the OP. MSDN says the return value is "The new value stored at *location1*." – dav_i Apr 03 '14 at 12:43
@dav_i You're right, I was mistaken on that ;) I retracted that bit of my comment. I was under the impression `Add` worked like `CompareExchange`, which does return the old value. – dcastro Apr 03 '14 at 12:44
@dcastro Now it looks like I'm just going mad! :P – dav_i Apr 03 '14 at 12:44

score 4 · Answer 3 · edited May 23 '17 at 12:06

4

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?

edited May 23 '17 at 12:06

Community

1
1

answered Apr 03 '14 at 11:56

Alex Erygin

3,161
1
22
22

This answer implies an *if and only if* relationship between use of non-local variables and thread safety. However, it is entirely possible to have thread-safe code which references externally scoped variables, through any variety of synchronization mechanisms. – Chris Hayes Apr 03 '14 at 19:18

Is this operation thread safe?

3 Answers3