ConcurrentDictionary - broken dictionary or bad code?

Question

Alright, so I'm running into a strange little issue and frankly I'm out of ideas. I wanted to throw this out there to see if I'm missing something that I've done wrong, or if ConcurrentDictionary isn't working correctly. Here's the code:

(Cache is a class containing the static ConcurrentDictionary Keys)

var tmp = Cache.Keys.GetOrAdd(type,
                key =>
                {
                    var keys = context.GetKeys(key);
                    if (keys.Count() == 1)
                    {
                        return new KeyInfo
                            {
                                Name = keys.First().Name,
                                Info = key.GetInfo(keys.First().Name)
                            };
                    }

                    return null;
                });

            if (tmp == null)
                Cache.Keys.TryRemove(type, out tmp);

            return tmp;

The problem is that occasionally tmp is null, causing the TryRemove line to run, yet the return null; line above is never hit. Since that return null is the only thing putting null into the dictionary and it never runs, how could tmp ever be null?

Including the Cache class (SetNames is not used by this code):

public class Cache
{
    public static ConcurrentDictionary<Type, Info> Keys = new ConcurrentDictionary<Type, Info>();
    public static ConcurrentDictionary<Type, string> SetNames = new ConcurrentDictionary<Type, string>();
}

How many threads are running this code? It doesn't seem to be thread-safe. — oleksii, Jan 05 '12 at 22:12
@IlyaKogan - No, the dictionary is empty when it starts and never contains a null, even when the breakpoint inside `if (tmp == null)` is hit. — Madeline Trotter, Jan 05 '12 at 22:14
@oleksii It was my every intent for it to be thread safe. Could you specify the areas you think will cause problems? — Madeline Trotter, Jan 05 '12 at 22:15
Are you sure that line is never hit? The breakpoint is set explicitly in the lambda and not on the statement itself? — James Michael Hare, Jan 05 '12 at 22:18
Also, can we see the Cache class? Are we sure nothing in it alters the `ConcurrentDictionary`? — James Michael Hare, Jan 05 '12 at 22:20
Is it possible that `context.GetKeys()` returns the same object as `Cache.Keys`? That would complicate things somewhat. — phoog, Jan 05 '12 at 22:20
@phoog It does not - `context.GetKeys()` returns a simple list, a new list for each call. — Madeline Trotter, Jan 05 '12 at 22:22
@JamesMichaelHare Unless it's broken, it's never hit. I've included `Cache` — Madeline Trotter, Jan 05 '12 at 22:24
@MichaelTrotter correct me if I am wrong. ConcurrentDictionary is thread safe, but the method GetOrAdd takes in a delegate Func. So if you have 2 threads that execute a delegate at the same time, as in the code, both threads would run this code concurrently with no synchronisation. This would be my guess. — oleksii, Jan 05 '12 at 22:31
@MichaelTrotter: Possible, but not likely. I've used ConcurrentDictionary a lot and never run into an issue like this, so I think it more likely there's something with the usage. — James Michael Hare, Jan 05 '12 at 22:34
@oleksii: you are correct, the call to the delegate may happen simultaneously for any given key, locking of the code inside of it (if desired) is the user's responsibility. I believe that was a design decision so that the CD wouldn't be hampered by a long factory method unless explicitly desired by the user. — James Michael Hare, Jan 05 '12 at 22:36
oleksii, that may happen, however I don't think it would cause any issue in this case. It might foul things up in the context.GetKeys method, yes, but I can't see how it might result in nulls in this Cache.Keys dictionary. — Chris Shain, Jan 05 '12 at 22:37
@oleksii True, but I'm not sure how that would be a problem. None of those threads are hitting `return null`, so while they may end up overriding each other on which instance of `KeyInfo` makes it into the dictionary, none of them could be putting `null` into it. — Madeline Trotter, Jan 05 '12 at 22:37
Tried replacing `return null` with `throw new Exception()` - none were thrown — Madeline Trotter, Jan 05 '12 at 22:43
Can you confirm that you are 100% sure that this code is what is actually running in your tests? Throw an exception in a *normal* code path- like in the delegate before getting the keys list. Make sure we are not chasing our tails. — Chris Shain, Jan 05 '12 at 23:25
The type returned by the factory is KeyInfo, the type stored is Info, is there a cast between them? — Jon Hanna, Jan 06 '12 at 12:42
@ChrisShain Presumably no, but it could be a copy-paste error rather than a matter of the actual code. — Jon Hanna, Jan 06 '12 at 20:13

score 3 · Answer 1 · edited Feb 22 '18 at 21:11

3

tmp can be null if you get anything other than a single item set back from context.GetKeys(key). In that case, keys.Count() != 1, and a null item will be inserted into Cache.Keys for the specified key (and returned from GetOrAdd, and assigned to tmp).

EDIT: Just thought of another possibility. What datatype is the key? Is it some kind of custom class? It looks like it is. If so, have you implemented Equals and GetHashcode properly?

edited Feb 22 '18 at 21:11

LeopardSkinPillBoxHat

28,915
15
75
111

answered Jan 05 '12 at 22:14

Chris Shain

50,833
6
93
125

but the OP states that the `return null` line is never executed, which, if correct, means that your scenario never occurs. – phoog Jan 05 '12 at 22:15
@phoog is correct - I also double checked the incoming parameters as well as `context.GetKeys(key)` and that case will not occur with this data. I expect it to happen in the future, but that's not the problem. – Madeline Trotter Jan 05 '12 at 22:17
Then the OP is mistaken :-) Either the dictionary contains a null for the specified key, or the return null line is executed. Now, from the variable names I am dubious about the call to GetOrAdd on the **Keys** member, but that's another story. – Chris Shain Jan 05 '12 at 22:17
@ChrisShain I understand what you're saying, and I've been trying to find out which of those is the case. The reason I created this post was because in every case neither of those are the problem, which doesn't make sense. If `Cache.Keys` ever contains a `null` then the breakpoint on `return null` is broken. – Madeline Trotter Jan 05 '12 at 22:20
@ChrisShain The key is a .net `Type` object – Madeline Trotter Jan 05 '12 at 22:25
Type implements Hashcode and Equals properly, so that isn't the issue... Can you put some logging in there rather than relying on the breakpoint? – Chris Shain Jan 05 '12 at 22:28
@ChrisShain That would be more reliable - working on that now – Madeline Trotter Jan 05 '12 at 22:39
Either return null is being hit in your value factory, or you are adding null to the dictionary somewhere else. Try changing return null into throw new exception. The you can be absolutely sure you are not adding nulls. – Jason Hernandez Jan 30 '12 at 20:30

score 1 · Accepted Answer · answered Dec 09 '13 at 19:16

I should have closed this a while ago but I completely forgot about it. The example is not thread safe because of TryRemove, but that was only added for debugging purposes. I ended up getting around the problem by rewriting it, so perhaps some of the comments about the code somehow being stale are correct. The code no longer exists to confirm, however.

I'm chalking this one up to user error (my own of course). Thanks for everyone's time!

ConcurrentDictionary - broken dictionary or bad code?

2 Answers2