ConcurrentDictionary adding same keys more than once

Question

I want to use ConcurrentDictionary to check if this data key has been added before, but it looks like I can still add keys which added before.

code:

    public class pKeys
    {
        public pKeys()
        { }
        public pKeys(long sID, long pID)
        {
            this.seID = sID;
            this.pgID = pID;

        }
        public long seID;
        public long pgID;
    }

    public static ConcurrentDictionary<pKeys, bool> existenceDic 
= new ConcurrentDictionary<pKeys, bool>();

test code:

    pKeys temKey = new pKeys(111, 222);
    bool res = existenceDic.TryAdd(temKey, true);
    Console.WriteLine(res);

    temKey = new pKeys(111, 222);
    res = existenceDic.TryAdd(temKey, true);
    Console.WriteLine(res);

result:

true
true

Your `pKeys` object doesn't override `GetHashCode()` or `Equals()`, so the two different instances are considered different values. Either use the same instance twice (instead of calling `new` the second time), or override those methods correctly so that different instances containing the same values are considered to be the same values. — Peter Duniho, Jan 17 '15 at 02:02
maybe easy fix , i will just convert the two element key into a string , then work like a hash.... — Benny Ae, Jan 17 '15 at 02:15
No, don't do that. If you do it that way, then the value of (11, 1222) will have the same hash code as the value of (111, 222), for no good reason. Collisions aren't fatal, but you should design the hash code implementation to at least try to avoid them as much as possible. — Peter Duniho, Jan 17 '15 at 03:34
`"111"+"="+"222"` is still wrong. It addresses only one of the three specific issues I pointed out with that particular approach (and frankly, the least important one). — Peter Duniho, Jan 18 '15 at 07:10

score 3 · Answer 1 · edited May 23 '17 at 12:12

You can add two different instances containing the same values, because you haven't overridden GetHashCode() and Equals(). This causes the default equality comparison to be used, which for reference types simply compares the references themselves. Two different instances are always considered as different values in this case.

One option is to make your type a struct instead of class. This uses a default comparison that will take into account the field values.

Alternatively, you can go ahead and override GetHashCode() and Equals(). For example:

public class pKeys
{
    public pKeys()
    { }
    public pKeys(long sID, long pID)
    {
        this.seID = sID;
        this.pgID = pID;

    }
    public readonly long seID;
    public readonly long pgID;

    public override int GetHashCode()
    {
        return seID.GetHashCode() * 37 + pgID.GetHashCode();
    }

    public override bool Equals(object other)
    {
        pKeys otherKeys = other as pKeys;

        return otherKeys != null &&
            this.seID == otherKeys.seID &&
            this.pgID == otherKeys.pgID;
    }
}

Notes:

The hash code is calculated based on the hash codes of the individual values. One is multiplied by 37, which is simply a convenient prime number; some people prefer to use a much larger prime number for better "mixing". For most scenarios, the above will work fine IMHO.
Note that your proposed solution, converting the values to strings, concatenating them, and returning the hash code of that has several negative aspects:
- You have to create three string instances just to generate the hash code! The memory overhead alone is bad enough, but of course there is the cost of formatting the two integers as well.
- Generating a hash code from a string is more expensive computationally than from an integer value
- You have a much higher risk of a collision, as it's easier for disparate values to result in the same string (e.g. (11, 2222) and (111, 222))
I added readonly to your fields. This would be critical if you decide to make the type a struct (i.e. even if you don't override the methods). But even for a class, mutable types that are equatable are a huge problem, because if they change after they are added to a hash-based collection, the collection is effectively corrupted. Using readonly here ensures that the type is immutable. (Also, IMHO public fields should be avoided, but if one must have them, they should definitely be readonly even if you don't override the equality methods).
Some people prefer to check for exact type equality in the Equals() method. In fact, this is often a good idea…it simplifies the scenarios where objects are compared and makes the code more maintainable. But for the sake of example, assignability (i.e. as) is easier to read, and is valid in many scenarios anyway.

See General advice and guidelines on how to properly override object.GetHashCode() for additional guidance.

ConcurrentDictionary adding same keys more than once

1 Answers1