1

I've met a very weird (for me..) Exception. It happens only rarely, but does...

My class isn't static, but has only one static attribute:

static Dictionary<string, ManualResetEvent> resetEvents = 
    new Dictionary<string, ManualResetEvent>();

When I'm trying to add for the first time a reset event - I'm getting, sometimes, a Null Reference Exception. Might this be related to two different Threads trying to add instance?

static ManualResetEvent resetEventsGet(string key)
{
    if (resetEvents.ContainsKey(key))
        return resetEvents[key];
    ManualResetEvent reste = new ManualResetEvent(false);
    resetEvents.Add(key, reste); //System.NullReferenceException: 'Object reference not set to an instance of an object.' HOW???
    return reste;
}

When I'm looking in the "watch" or immediate window there's no null anywhere (the dictionary or the resetEvent).

p.s - I tagged it for visual studio 2017 because it never happened to me before, although code didn't change. Any idea? Thanks

ephraim
  • 379
  • 1
  • 3
  • 15
  • That is weird, since it seem tot get past the `ContainsKey` call. You don't have `resetEvents = null` anywhere do you? – juharr Mar 20 '17 at 15:04
  • It doesn't make sense to me. Why the exception isn't thrown in the line `if (resetEvents.ContainsKey(key))`? Are you sure you are not making that field `null` from any where else in the code? – dcg Mar 20 '17 at 15:04
  • What does the call to `new ManualResetEvent(false)` do? It does not make the field `null`? – Marcello Mar 20 '17 at 15:06
  • "When I'm trying to add for the first time a reset event" - that sounds like you can reproduce this. So please provide a [mcve]. – Jon Skeet Mar 20 '17 at 15:06
  • 5
    `Dictionary` is not thread-safe. If you access it from multiple threads without using locks, you can get odd results including an NRE – Joe Mar 20 '17 at 15:06
  • Totally sure. Checked ten times...and now again, the dictionary has only three references - all in front of you (declaration, containsKey and add) – ephraim Mar 20 '17 at 15:07
  • @Joe I mentioned such an option in the question, but can't understand how this may happen. Could you please explain more? Thanks – ephraim Mar 20 '17 at 15:09
  • 1
    @UweKeim not all questions that contain words NullReferenceException are duplicates of that one. – Evk Mar 20 '17 at 15:11

2 Answers2

4

That's perfectly possible if you call resetEventsGet from multiple threads. Dictionary.Add is not thread safe, and when you call it from multiple threads - weird things might happen, which includes throwing 'NullReferenceException'. It's relatively easy to reproduce with the following code:

class Program {
    static Dictionary<string, ManualResetEvent> resetEvents = new Dictionary<string, ManualResetEvent>();

    static void Main()
    {
        for (int i = 0; i < 1000; i++) {
            new Thread(() =>
            {
                resetEvents.Add(Guid.NewGuid().ToString(), new ManualResetEvent(false));
            })
            {
                IsBackground = true
            }.Start();
        }
        Console.ReadKey();
    }      
}

This code not always, but very often, throws null reference exception inside Dictionary.Insert private method.

This happens because dictionary stores your values in array-like internal structures, and those structures are not of fixed size. When you add more values - dictionary might resize it's internal structures, and that resize might happen when another thread already enumerates them at the same time. Doing resize and enumeration at the same time might lead to many bad things, including null reference or index out of range exceptions.

So just don't ever do it and use proper locking. Or use collections that are designed for multithreaded access, like ConcurrentDictionary<string, ManualResetEvent>.

Evk
  • 98,527
  • 8
  • 141
  • 191
  • Thanks! finally managed to test - Actually I didn't have any exceptions on visual studio 2008 (framWork 3.5). on 2017 I had very few (after initiating array of Guid prior loop) , aside to some keyValues which just didn't enter (tried to fill with 1000 and found only 998...without any exception). Intresting. Thanks again. Obviously I add lock\ConcurrentDictionary now. – ephraim Mar 20 '17 at 21:24
  • Ha!! finally it happened to me also in VS 2008... I think the actual difference was I never run VS 2008 on release mode, but 2017 I do... Now it happend on 2008 on release mode. – ephraim Jun 04 '17 at 05:28
3

If you are accessing this with multiple threads, you'd better lock it. The problem is, that the Dictionary isn't threadsafe. In this case, you can use the Dictionary itself as lockobject. (because it is private)

Something like:

static ManualResetEvent resetEventsGet(string key)
{
    lock(resetEvents)
    {
        ManualResetEvent result;

        // lookup the key
        if(!resetEvents.TryGetValue(key, out result))
            // if it doesn't exists, create a new one.
            resetEvents.Add(key, result = new ManualResetEvent(false));

        return result;
    }
}

Also TryGetValue is great. It is giving you the value and if it was present. (so only one lookup instead of two)

Jeroen van Langen
  • 21,446
  • 3
  • 42
  • 57