Null reference in web api call

Question

This is a weird one, and I'm at a loss to understand what's going on here. I have a web api project that in one controller a call to a certain method eventually calls a function in a service that looks like this:

    public MyClassBase GetThing(Guid id)
    {
        if (cache.ContainsKey(id))
        {
            return cache[id];
        }
        else
        {
            var type = typeof(MyClassBase).Assembly.GetTypes().FirstOrDefault
            (
                t => t.IsClass &&
                    t.Namespace == typeof(MyClassBase).Namespace + ".Foo" &&
                    t.IsSubclassOf(typeof(MyClassBase)) &&
                    (t.GetCustomAttribute<MyIdAttribute>()).GUID == id
            );
            if (type != null)
            {
                System.Diagnostics.Debug.WriteLine(string.Format("Cache null: {0}",cache == null));
                var param = (MyClassBase)Activator.CreateInstance(type, userService);
                cache[id] = param;
                return param;
            }
            return null;
        }
    }

cache is simply a dictionary:

 protected Dictionary<Guid, MyClassBase> cache { get; set; }

That gets created in the constructor for this class:

 cache = new Dictionary<Guid, MyClassBase>();

This works perfectly 99.9% of the time, but occasionally, when first starting up the app, the first request will throw a NullReferenceException - and the weird part is, it claims that the source is this line:

cache[id] = param;

But the thing is, if cache is null (which is can't be, it was set in the constructor, it's private, and this is the only method that even touches it), then it should have thrown at:

if (cache.ContainsKey(id))

and if id was null, then I would have got a bad request from the api because it wouldn't map, plus my linq statement to get the type with a matching GUID would have returned null, which I'm also testing for. And if param is null, it shouldn't matter, you can set a dictionary entry to a null value.

It feels like a race condition with something not being fully initialized, but I can't see where it's coming from or how to defend against it.

Here's an example of what it (occassionally) throws (as JSON because the web api returns json and I've current got it spitting me back error messages so I can find them):

{
    "message": "An error has occurred.",
    "exceptionMessage": "Object reference not set to an instance of an object.",
    "exceptionType": "System.NullReferenceException",
    "stackTrace": "   at System.Collections.Generic.Dictionary`2.Insert(TKey key, 
        TValue value, Boolean add)\r\n   at 
        System.Collections.Generic.Dictionary`2.set_Item(TKey key, TValue value)\r\n   
        at MyNameSpace.Services.MyFinderService.GetThing(Guid id) in
        c:\\...\\MyFinderService.cs:line 85\r\n   
        at MyNameSpace.Controllers.MyController.GetMyParameters(Guid id) in 
        c:\\...\\Controllers\\MyController.cs:line 28\r\n   at 
        lambda_method(Closure , Object , Object[] )\r\n   at
        System.Web.Http.Controllers.ReflectedHttpActionDescriptor.ActionExecutor.<>c__DisplayClass13.
        <GetExecutor>b__c(Object instance, Object[] methodParameters)\r\n   at
        System.Web.Http.Controllers.ReflectedHttpActionDescriptor.ActionExecutor.Execute(Object instance,
        Object[] arguments)\r\n   at System.Web.Http.Controllers.ReflectedHttpActionDescriptor.
        <>c__DisplayClass5.<ExecuteAsync>b__4()\r\n   at
        System.Threading.Tasks.TaskHelpers.RunSynchronously[TResult](Func`1 func, CancellationToken
        cancellationToken)"
}

The line 85 is the line I highlighted above.

It's kind of a Heisenbug, but I did just manage to get it to throw by doing this immediately on my html page (of course, doing it a second time it worked just fine):

    $.ajax({
        url: "@Url.Content("~/api/MyController/GetMyParameters")",
    data: { id: '124c5a71-65b7-4abd-97c0-f5a7cf1c4150' },
    type: "GET"
    }).done(function () { console.log("done"); }).fail(function () { console.log("failed") });

    $.ajax({
        url: "@Url.Content("~/api/MyController/GetMyParameters")",
        data: { id: '7cc9d80c-e7c7-4205-9b0d-4e6cb8adbb57' },
    type: "GET"
    }).done(function () { console.log("done"); }).fail(function () { console.log("failed") });

    $.ajax({
        url: "@Url.Content("~/api/MyController/GetMyParameters")",
        data: { id: '1ea79444-5fae-429c-aabd-2075d95a1032' },
    type: "GET"
    }).done(function () { console.log("done"); }).fail(function () { console.log("failed") });

    $.ajax({
        url: "@Url.Content("~/api/MyController/GetMyParameters")",
        data: { id: 'cede07f3-4180-44fe-843b-f0132e3ccebe' },
    type: "GET"
    }).done(function() { console.log("done"); }).fail(function() { console.log("failed")});

Firing off four requests in quick succession, two of them failed at the same point, but here's where it gets really infuriating, it breaks on that line and I can hover over cache, id and param in Visual Studio and see their current value and none of them are null! And in each case cache has a Count of 1, which is also odd because this is supposed to be a singleton created by Ninject, so I'm starting to suspect something screwy with Ninject.

For reference, in NinjectWebCommon, I am registering this service like this:

kernel.Bind<Services.IMyFinderService>().To<Services.MyFinderService>().InSingletonScope();

Note: I also posted this Singletons and race conditions because I'm not 100% sure the problem isn't Ninject, but I didn't want to confuse this question with too many conjectures about what might be the cause.

Answer 1

The problem is that Dictionary is not thread-safe.

Look at the stack trace, the exception is thrown inside Dictionary's internal Insert method. So cache is definitely not null. id can't be null either, because Guid is a value type. Null values are allowed in a dictionary so it doesn't matter if param is null. The problem is that one thread is probably in the middle of an update that caused the dictionary to reallocate its internal buckets while another thread goes tries to insert another value. Some internal state is inconsistent and it throws. I'd bet this happens once in a while during initialization because that's when caches tend to get filled.

You need to make this class thread-safe which means locking access to the dictionary or (even easier) using a class like ConcurrentDictionary that is designed to be thread-safe.