Is this code thread safe in ASP.NET MVC 4?

Question

Based on my current understanding I do not think this code is thread safe but want to confirm. In other words I think that, although it would be extremely unlikely, multiple threads representing different HTTP requests could potentially mix up the value of the _userName property.

public class SomeClass
{
    private static string _userName;

    public static string UserName
    {
        get
        {
            if (string.IsNullOrEmpty(_userName))
            {
                _userName = HttpContext.Current.User.Identity.Name;
            }

            return _userName; 
        }
    }
}

Is it thread-safe and if not would removing the null check, and always accessing HttpContext.Current.User.Identity.Name directly (in a static property) be thread-safe?

public class SomeClass
{
    public static string UserName
    {
        get
        {
            return HttpContext.Current.User.Identity.Name; 
        }
    }
}

Correct, your first example is not thread-safe because static members are shared between threads (unless you're using thread-local storage, which you're not), and the `HttpContext.Current` property **is** stored in thread-local storage. — Dai, Feb 19 '15 at 23:02
Also, there's a second thread-safety issue: your thread could be preempted between the `if` and `=` statements so you'd want a `lock`, not that he code is a good idea in the first place. — Dai, Feb 19 '15 at 23:03
"although it would be extremely unlikely" It's probably much more likely than you think. — hatchet - done with SOverflow, Feb 19 '15 at 23:04

D Stanley · Answer 1 · 2015-02-19T23:22:52.517

3

Your two examples are very different. You are right in that your first example is not thread safe, but the more important problem is that it's not session-safe. The first session that accesses UserName will set the username, and all other sessions will use that same name! UserName will not change until the App pool is recycled. Does it matter that technically the second session set the user name if it made the request just slightly after the first?

If you want to cache the user name for each session then use the Session property:

Session["UserName"] = HttpContext.Current.User.Identity.Name;

The second block is thread-safe, but it returns the current user name every time it's called. So there's no worry about data getting crossed across threads, let alone sessions.

edited Feb 19 '15 at 23:22

answered Feb 19 '15 at 23:14

D Stanley

149,601
11
178
240

"UserName will not change until the App pool is recycled." How sure are you about this part? That doesn't seem to be how it behaves but I'll look into it more. I am thinking it would come down to when the implicit static constructor is called for the class -- wouldn't the static constructor clear _userName every time it is called? Is it called only once for the life of the process? – Sean Thoman Feb 19 '15 at 23:28
1

@SeanThoman static properties are static for the whole app pool. The first time someone tries to read the `UserName` property the current session's user is extracted and the static field is set - unless you have other code that changes that field it will stay the same value until the process dies. – D Stanley Feb 20 '15 at 13:50

score 2 · Accepted Answer · edited May 23 '17 at 12:14

Your second solution would work fine. The first one is not thread safe at all. Caching a value in a static variable exposes it to all threads, and if you have two or more simultaneous requests, it's highly likely they will read some other request's value. You think it's extremely unlikely, well... it's not, quite the opposite.

Besides, if you really want a value to be shared across multiple threads, you should use some synchronization mechanism to ensure correctness. In your first example, you access _userName 3 times (2 reads and 1 write). You can get into a situation where this variable ends up different values, so you should enclose everything in a lock. And that's only when you explicitly want to share this information between threads.

The second approach works because the value of HttpContext.Current is retrieved from the current execution context (so it's dependent on the current thread).

Oh one more thing: in an ASP.NET application you shouldn't rely on thread-local storage because of thread agility, so if you need to cache values use the HttpContext instead. You have an Items property which lets you store values for the duration of the current request only. but in your particular example, you don't have to cache this value, because it's already retrieved from the HTTP context.

Is this code thread safe in ASP.NET MVC 4?

2 Answers2