4

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.

Community
  • 1
  • 1
Matt Burland
  • 44,552
  • 18
  • 99
  • 171
  • 2
    Are you sure it's a `NullReferenceException` and not an `ArgumentNullException`? If `id` is `null` when calling `cache[id]`, you will get an `ArgumentNullException`. http://msdn.microsoft.com/en-us/library/9tee9ht2.aspx – Evan Mulawski May 01 '14 at 13:48
  • I would suspect this is being thrown because userService is not defined, causing the activator to fail, and resulting in param being undefined in the line throwing the error. – theDarse May 01 '14 at 13:58
  • @EvanMulawski: No, it's definitely a `NullReferenceException`. I added an example above. – Matt Burland May 01 '14 at 14:00
  • @theDarse: I suspected that too (especially since `userService` is injected by ninject to the constructor), but it doesn't seem to be the case. First, I'd expect the previous line to throw if it couldn't construct it. Second, I checked every class that can be constructed by this code and the only thing they do with the `userService` argument is store it in a variable to be used later. So passing null to them should be an issue - at least not yet. And third, `myDictionary[someKey] = null` is perfectly valid code anyway. – Matt Burland May 01 '14 at 14:03
  • @MattBurland: Then `cache` is definitely `null`. Are you running the API locally? If so, you can run+debug it and set breakpoints. I would set a breakpoint at Line 85 and after you initialize `cache` in the constructor. – Evan Mulawski May 01 '14 at 14:11
  • If `cache.ContainsKey(id)` does indeed return true during your initial check, then you'll never even reach the point of setting `cache[id] = param`. Try adding a `try/catch` around that set statement and breakpoint in the catch. Or write to a log in the catch if the code is in production so you can see which id is the problem. – McCee May 01 '14 at 14:11
  • @EvanMulawski: Well, that's the thing, `cache` must be `null` for that error to occur, but if it is, then the `if` statement above would have been `if (null.ConatinsKey(id))` and *that* should have been my `NullReferenceException`. I shouldn't even be able to reach that line if `cache` is null. – Matt Burland May 01 '14 at 14:21

1 Answers1

3

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.

Mike Zboray
  • 39,828
  • 3
  • 90
  • 122
  • Holy crap - That's one of those answers that you immediately know *has to be right*. I just tested it probably a dozen times and it didn't throw anything yet. Thank you. – Matt Burland May 01 '14 at 16:48
  • Also, that's a really great point that the stack trace was *telling me* that the error was coming from `Dictionary` internally. I really should have spotted that. – Matt Burland May 01 '14 at 16:53