Static List multithreaded application

Question

public class Helper
{
    private static List<TimeZoneMap> timeZoneList = null;


public static List<TimeZoneMap> GetTimeZoneList()
{
    if (timeZoneList == null)
    {
        timeZoneList = new List<TimeZoneMap>();
        timeZoneList.Add(new TimeZoneMap() { Id = "Dateline Standard Time", DisplayName = "(UTC-12:00) International Date Line West" });
        timeZoneList.Add(new TimeZoneMap() { Id = "UTC-11", DisplayName = "(UTC-11:00) Coordinated Universal Time-11" });
        timeZoneList.Add(new TimeZoneMap() { Id = "Aleutian Standard Time", DisplayName = "(UTC-10:00) Aleutian Islands" });
    }
}

The timeZoneList gets initialized inside an instance class (Helper) but it is static a list.

A static method add items to this list based on null check.

In a multi threaded environment, the timeZoneList does not have all the values and return nulls randomly. This occurs for only 2 to 3 random instance out of the 200.

What we are doing wrong here ?

1. Static List
2. Static Method
3. An Instance class initialization inside the static list ?
4. or calling the same static method from different instances of parent class. ?

This is not frequently happening and cannot guess when the issue occurs as well.

Thinking of making this list as static readonly collection. But still doubt that would make the list read only. What on the initial load we get only 5 instead of all !?

Answer 1

If multiple threads call GetTimeZoneList() at the same time while timeZoneList is null, you'll get a race condition. The fact that it is static is irrelevant here.

The reason for this is that, if, say, two threads enter the method at the same time while the list is null, they'll both enter the if at the same time. That is bad, because now you have concurrent writes to the timeZoneList field, concurrent modification to a non thread-safe collection, or both. Ultimately, your problem is that you're sharing mutable states between threads, and that is always bad.

There's also the other issue of using a mutable collection (List<T>) for something that might be shared between threads, which creates the risk that some part of the code modifies it afterwards, wreaking further havoc. I recommend using something like ImmutableArray<T> instead:

var builder = ImmutableArray.CreateBuilder<TimeZoneMap>();
builder.Add(new TimeZoneMap() {
    Id = "Dateline Standard Time",
    DisplayName = "(UTC-12:00) International Date Line West" 
});
builder.Add(new TimeZoneMap() {
    Id = "UTC-11",
    DisplayName = "(UTC-11:00) Coordinated Universal Time-11" 
});
builder.Add(new TimeZoneMap() { 
    Id = "Aleutian Standard Time", 
    DisplayName = "(UTC-10:00) Aleutian Islands" 
});}
return builder.ToImmutable();

Once the ImmutableArray<T> is constructed, it cannot be changed, preventing other problems. You can then choose to return IReadOnlyCollection<T> or IReadOnlyList<T> to hide that implementation detail.

Let's look at your options.

1. Make it immutable

Initialisation of a static field is guaranteed to happen only once, so this is inherently thread safe:

private static readonly ImmutableArray<TimeZoneMap> timeZoneList 
    = CreateTimeZoneList();

private static ImmutableArray<TimeZoneMap> CreateTimeZoneList()
{
    // create, fill and return the list
}

public static IReadOnlyList<TimeZoneMap> GetTimeZoneList() 
    => timeZoneList;

It does have the issue of making the initialisation eager, which might not be what you want. To keep it lazy, read on.

2. Lock it

Use the lock statement with an object to control access to the field:

private static ImmutableArray<TimeZoneMap> timeZoneList = null;
private static readonly object timeZoneLock = new object();

public static IReadOnlyList<TimeZoneMap> GetTimeZoneList()
{
    lock(timeZoneLock)
    {
        if (timeZoneList == null)
        {
            // etc.
        }
    }
    return timeZoneList;
}

That's fairly straightforward, but a bit verbose. Also, it looks like a singleton, and I don't like singletons. You could also do another null check outside of the lock to avoid locking it unnecessarily, but that's an optimisation. Up to you.

Use Lazy<T>

For me, this is the best approach: it's simpler, leaner, and more robust.

Lazy<T> is a class that, as its name implies, contains a value that is lazily initialised. By default, this object performs its initialisation in a thread safe manner (specifically, with the ExecutionAndPublication thread safety mode), so, you could do this:

private static readonly Lazy<ImmutableArray<TimeZoneMap>> timeZoneList 
    = new Lazy<ImmutableArray<TimeZoneMap>>(CreateTimeZoneList);

private static ImmutableArray<TimeZoneMap> CreateTimeZoneList()
{
    // ...
}

public static IReadOnlyList<TimeZoneMap> GetTimeZoneList() 
    => timeZoneList.Value;

The above eagerly creates the lazy object, specifying the method to use as its factory. The list is initially not created. The first time some gets the lazy's Value (by calling GetTimeZoneList()), it will then be properly initialised. Subsequent calls return the same list. If multiple thread do so at the same time, the factory method will still be called only once, and all threads then get the same exact list.