C# variable that should never be null throws null reference exception

Question

I have find a currious situation, that I can't explain. Basicaly property "UserCalls" works fine at the begining, but after long time since start of application (more than a month), property that should never take null value, throws null exception when functions are called on it (eg. linq .FirstOrDefault() ).

Code example:

public class UserCall
{
    public long UserID { get; set; }
    public long CallID { get; set; }
}

public static class Cache
{
    private static List<UserCall> userCalls = new List<UserCall>();

    public static List<UserCall> UserCalls
    {
        get
        {
            if (userCalls == null)
            {
                userCalls = new List<UserCall>();
            }

            return userCalls;
        }

        set
        {
            userCalls = value;
        }
    }

    private static void AddCall(long userID, long callID)
    {
        UserCalls.Add(
                    new UserCall
                    {
                        CallID = callID,
                        UserID = userID
                    });
    }


    public static UserCall GetCall(long userID)
    {
        var userCall = UserCalls.FirstOrDefault(x => x.UserID == userID);
        return userCall;
    }

    public static void RemoveCall(long userID)
    {
        UserCalls.RemoveAll(x => x.UserID == userID);
    }
}

The following exception is thrown when calling "GetCall":

    [ERROR] Object reference not set to an instance of an object. | at System.Linq.Enumerable.FirstOrDefault[TSource](IEnumerable`1 source, Func`2 predicate)

Methods "AddCall", "GetCall" and "RemoveCall" can be called from multiple threads. This exeception happens at random (but only after long time since service started) on more loaded services. If exception occurs, it is locked in this permanent null state and will not correct itself, only service restart will help. I am aware that this implementation is not exactly thread safe, but still it should never return null. Why does it happen? Have anyone met with similar situation ?

My first thought looking at your code is that the `UserCalls` member is not thread-safe, and that _could_ explain the inconsistent behaviour. — Martin, Sep 27 '19 at 08:58
a public setter on a list is allmost allways a bad idea, as it enables users to modify the list arbitrarily. In particular it allows both `UserCalls = null` as well as `UserCalls.Add(null)`. — MakePeaceGreatAgain, Sep 27 '19 at 08:58
What is the purpose of `set { userCalls = value; }`? Why do you allow to set `null` at all? Another issue: `List` is not thread safe, why not turn it into `ConcurrentQueue`? — Dmitry Bychenko, Sep 27 '19 at 08:58
I think it's work. But if you try to use return of Cache.GetCall method. You have to check null cause maybe return null. — Gürol Mehmet Çetin, Sep 27 '19 at 09:05
Even if set to null, wouldn't next get set it to new instance? — Monkey, Sep 27 '19 at 09:15
The exception looks like there is a null value inside the UserCall list rather than it being null — Ventsyslav Raikov, Sep 27 '19 at 09:22
Possible duplicate of [What is a NullReferenceException, and how do I fix it?](https://stackoverflow.com/questions/4660142/what-is-a-nullreferenceexception-and-how-do-i-fix-it) — Stefan, Sep 27 '19 at 09:25

Matthew Watson · Accepted Answer · 2019-09-27T09:21:08.740

Your UserCalls property can be null in a multithreaded context.

Given:

public static List<UserCall> UserCalls
{
    get
    {
        if (userCalls == null)
        {
            userCalls = new List<UserCall>();
        }

        return userCalls;
    }

    set
    {
        userCalls = value;
    }
}

Consider the following sequence with two threads, [A] and [B] where userCalls is currently non-null.

[A] Accesses `UserCalls` getter. 
[A] Checks if `userCalls` is null. It is not, so it will skip the assignment.    
[B] Accesses `UserCalls` setter, about to set it to `null`.
[B] Sets `userCalls` to null.
[A] Returns `userCalls`, which is now null => BANG!

You can protect against this by using a lock:

public static List<UserCall> UserCalls
{
    get
    {
        lock (_locker)
        {
            if (userCalls == null)
            {
                userCalls = new List<UserCall>();
            }

            return userCalls;
        }
    }

    set
    {
        lock (_locker)
        { 
            userCalls = value;
        }
    }
}

static object _locker = new object();

However, note that there is nothing to prevent something from setting one of the elements of UserCalls to null. If something does that, you will also get a NullReferenceException in calls like UserCalls.FirstOrDefault(x => x.UserID == userID);, since x would be null and using x.UserID will give you the null reference exception.

I think that might be the case, there might be null objects inside list, thanks — Monkey, Sep 27 '19 at 09:45

Stefan · Answer 2 · 2019-09-27T11:09:15.093

I'd suggest:

Remove property UserCalls and use the list only as field. This prevents the possibility of adding a null value or the list being modified by some other process than your own.
List item

Add a lock to make the usage of the list thread safe

The code:

public static class Cache
{
    private static List<UserCall> userCalls = new List<UserCall>();    

    private static void AddCall(long userID, long callID)
    {
        lock(userCalls)
        {
            userCalls.Add(
                    new UserCall
                    {
                        CallID = callID,
                        UserID = userID
                    });
        }
    }


    public static UserCall GetCall(long userID)
    {
        UserCall userCall = null;
        lock(userCalls)
            userCall = UserCalls.FirstOrDefault(x => x.UserID == userID);
        return userCall;
    }

    public static void RemoveCall(long userID)
    {
        lock(userCalls)
            userCalls.RemoveAll(x => x.UserID == userID);
    }
}

C# variable that should never be null throws null reference exception

2 Answers2