Duplicate keys in Dictionary when using PhysicalAddress as the key

Question

So I have run into an interesting problem where I am getting duplicate keys in C# Dictionary when using a key of type PhysicalAddress. It is interesting because it only happens after a very long period of time, and I cannot reproduce it using the same code in a unit test on a completely different machine. I can reproduce it reliably on a Windows XP SP3 machine but only after letting it run for days at a time, and even then it only occurs once.

Below is the code that I am using and beneath that is the log output for that part of the code.

Code:

private void ProcessMessages()
{
    IDictionary<PhysicalAddress, TagData> displayableTags = new Dictionary<PhysicalAddress, TagData>();

    while (true)
    {
        try
        {
            var message = incomingMessages.Take(cancellationToken.Token);

            VipTagsDisappeared tagsDisappeared = message as VipTagsDisappeared;

            if (message is VipTagsDisappeared)
            {
                foreach (var tag in tagDataRepository.GetFromTagReports(tagsDisappeared.Tags))
                {
                    log.DebugFormat(CultureInfo.InvariantCulture, "Lost tag {0}", tag);

                    RemoveTag(tag, displayableTags);
                }

                LogKeysAndValues(displayableTags);

                PublishCurrentDisplayableTags(displayableTags);
            }
            else if (message is ClearAllTags)
            {
                displayableTags.Clear();
                eventAggregator.Publish(new TagReaderError());
            }
            else if (message is VipTagsAppeared)
            {
                foreach (TagData tag in tagDataRepository.GetFromTagReports(message.Tags))
                {
                    log.DebugFormat(CultureInfo.InvariantCulture, "Detected tag ({0}) with Exciter Id ({1})", tag.MacAddress, tag.ExciterId);

                    if (tagRules.IsTagRssiWithinThreshold(tag) && tagRules.IsTagExciterValid(tag))
                    {
                        log.DebugFormat(CultureInfo.InvariantCulture, "Detected tag is displayable ({0})", tag);

                        bool elementAlreadyExists = displayableTags.ContainsKey(tag.MacAddress);

                        if (elementAlreadyExists)
                        {
                            displayableTags[tag.MacAddress].Rssi = tag.Rssi;
                        }
                        else
                        {
                            displayableTags.Add(tag.MacAddress, tag);
                        }
                    }
                    else
                    {
                        log.DebugFormat(CultureInfo.InvariantCulture, "Detected tag is not displayable ({0})", tag);

                        RemoveTag(tag, displayableTags);
                    }
                }

                LogKeysAndValues(displayableTags);

                PublishCurrentDisplayableTags(displayableTags);
            }
            else
            {
                log.WarnFormat(CultureInfo.InvariantCulture, "Received message of unknown type {0}.", message.GetType());
            }
        }
        catch (OperationCanceledException)
        {
            break;
        }
    }
}

private void PublishCurrentDisplayableTags(IDictionary<PhysicalAddress, TagData> displayableTags)
{
    eventAggregator.Publish(new CurrentDisplayableTags(displayableTags.Values.Distinct().ToList()));
}

private void RemoveTag(TagData tag, IDictionary<PhysicalAddress, TagData> displayableTags)
{
    displayableTags.Remove(tag.MacAddress);

    // Now try to remove any duplicates and if there are then log it out
    bool removalWasSuccesful = displayableTags.Remove(tag.MacAddress);

    while (removalWasSuccesful)
    {
        log.WarnFormat(CultureInfo.InvariantCulture, "Duplicate tag removed from dictionary: {0}", tag.MacAddress);
        removalWasSuccesful = displayableTags.Remove(tag.MacAddress);
    }
}

private void LogKeysAndValues(IDictionary<PhysicalAddress, TagData> displayableTags)
{
    log.TraceFormat(CultureInfo.InvariantCulture, "Keys");
    foreach (var physicalAddress in displayableTags.Keys)
    {
        log.TraceFormat(CultureInfo.InvariantCulture, "Address: {0}", physicalAddress);
    }

    log.TraceFormat(CultureInfo.InvariantCulture, "Values");
    foreach (TagData physicalAddress in displayableTags.Values)
    {
        log.TraceFormat(CultureInfo.InvariantCulture, "Address: {0} Name: {1}", physicalAddress.MacAddress, physicalAddress.Name);
    }
}

And process messages is used as follows:

Thread processingThread = new Thread(ProcessMessages);

GetFromTagReports Code

public IEnumerable<TagData> GetFromTagReports(IEnumerable<TagReport> tagReports)
{
    foreach (var tagReport in tagReports)
    {
        TagData tagData = GetFromMacAddress(tagReport.MacAddress);
        tagData.Rssi = tagReport.ReceivedSignalStrength;
        tagData.ExciterId = tagReport.ExciterId;
        tagData.MacAddress = tagReport.MacAddress;
        tagData.Arrived = tagReport.TimeStamp;

        yield return tagData;
    }
}

public TagData GetFromMacAddress(PhysicalAddress macAddress)
{
    TagId physicalAddressToTagId = TagId.Parse(macAddress);

    var personEntity = personFinder.ByTagId(physicalAddressToTagId);

    if (personEntity.Person != null && !(personEntity.Person is UnknownPerson))
    {
        return new TagData(TagType.Person, personEntity.Person.Name);
    }

    var tagEntity = tagFinder.ByTagId(physicalAddressToTagId);

    if (TagId.Invalid == tagEntity.Tag)
    {
        return TagData.CreateUnknownTagData(macAddress);
    }

    var equipmentEntity = equipmentFinder.ById(tagEntity.MineSuiteId);

    if (equipmentEntity.Equipment != null && !(equipmentEntity.Equipment is UnknownEquipment))
    {
        return new TagData(TagType.Vehicle, equipmentEntity.Equipment.Name);
    }

    return TagData.CreateUnknownTagData(macAddress);
}

Where Physical Address is created

var physicalAddressBytes = new byte[6];
ByteWriter.WriteBytesToBuffer(physicalAddressBytes, 0, protocolDataUnit.Payload, 4, 6);

var args = new TagReport
{
    Version = protocolDataUnit.Version,
    MacAddress = new PhysicalAddress(physicalAddressBytes),
    BatteryStatus = protocolDataUnit.Payload[10],
    ReceivedSignalStrength = IPAddress.NetworkToHostOrder(BitConverter.ToInt16(protocolDataUnit.Payload, 12)),
    ExciterId = IPAddress.NetworkToHostOrder(BitConverter.ToInt16(protocolDataUnit.Payload, 14))
};

public static void WriteBytesToBuffer(byte[] oldValues, int oldValuesStartindex, byte[] newValues, int newValuesStartindex, int max)
{
    var loopmax = (max > newValues.Length || max < 0) ? newValues.Length : max;

    for (int i = 0; i < loopmax; ++i)
    {
        oldValues[oldValuesStartindex + i] = newValues[newValuesStartindex + i];
    }
}

Note the following:

• Every 'Tag' in messages.Tags contains a 'new' PhysicalAddress.
• Each TagData that is returned is also 'new'.
• The 'tagRules' methods do not modify the passed in 'tag' in any way.
• Individual testing with trying to put two instances of a PhysicalAddress (that were created from the same bytes) into a Dictionary throws a 'KeyAlreadyExists' exception.
• I also tried TryGetValue and it produced the same result.

Log output where everything was fine:

2013-04-26 18:28:34,347 [8] DEBUG ClassName - Detected tag (000CCC756081) with Exciter Id (0)
2013-04-26 18:28:34,347 [8] DEBUG ClassName - Detected tag is displayable (Unknown: ?56081)
2013-04-26 18:28:34,347 [8] TRACE ClassName - Keys
2013-04-26 18:28:34,347 [8] TRACE ClassName - Address: 000CCC755898
2013-04-26 18:28:34,347 [8] TRACE ClassName - Address: 000CCC756081
2013-04-26 18:28:34,347 [8] TRACE ClassName - Address: 000CCC755A27
2013-04-26 18:28:34,347 [8] TRACE ClassName - Address: 000CCC755B47
2013-04-26 18:28:34,347 [8] TRACE ClassName - Values
2013-04-26 18:28:34,347 [8] TRACE ClassName - Address: 000CCC755898 Name: Scotty McTester
2013-04-26 18:28:34,347 [8] TRACE ClassName - Address: 000CCC756081 Name: ?56081
2013-04-26 18:28:34,347 [8] TRACE ClassName - Address: 000CCC755A27 Name: JDTest1
2013-04-26 18:28:34,347 [8] TRACE ClassName - Address: 000CCC755B47 Name: 33 1
2013-04-26 18:28:34,347 [8] TRACE ClassName - Current tags: Scotty McTester, ?56081, JDTest1, 33 1

Log output where we get a duplicate key:

2013-04-26 18:28:35,608 [8] DEBUG ClassName - Detected tag (000CCC756081) with Exciter Id (0)
2013-04-26 18:28:35,608 [8] DEBUG ClassName - Detected tag is displayable (Unknown: ?56081)
2013-04-26 18:28:35,608 [8] TRACE ClassName - Keys
2013-04-26 18:28:35,608 [8] TRACE ClassName - Address: 000CCC755898
2013-04-26 18:28:35,608 [8] TRACE ClassName - Address: 000CCC756081
2013-04-26 18:28:35,618 [8] TRACE ClassName - Address: 000CCC755A27
2013-04-26 18:28:35,618 [8] TRACE ClassName - Address: 000CCC755B47
2013-04-26 18:28:35,618 [8] TRACE ClassName - Address: 000CCC756081
2013-04-26 18:28:35,618 [8] TRACE ClassName - Values
2013-04-26 18:28:35,618 [8] TRACE ClassName - Address: 000CCC755898 Name: Scotty McTester
2013-04-26 18:28:35,618 [8] TRACE ClassName - Address: 000CCC756081 Name: ?56081
2013-04-26 18:28:35,648 [8] TRACE ClassName - Address: 000CCC755A27 Name: JDTest1
2013-04-26 18:28:35,648 [8] TRACE ClassName - Address: 000CCC755B47 Name: 33 1
2013-04-26 18:28:35,648 [8] TRACE ClassName - Address: 000CCC756081 Name: ?56081
2013-04-26 18:28:35,648 [8] TRACE ClassName - Current tags: Scotty McTester, ?56081, JDTest1, 33 1, ?56081

Notice that everything is happening on a single thread (see the [8]) so there is no chance of the dictionary having been concurrently modified. The excerpts are from the same log and the same process instance. Also notice that in the second set of logs we end up with two keys that are the same!

What I am looking into: I have changed PhysicalAddress to a string to see if I can eliminate that from the list of suspects.

My questions are:

• Is there a problem that I'm not seeing in the code above?
• Is there a problem with the equality methods on PhysicalAddress? (That only error every now and then?)
• Is there a problem with the Dictionary?

Answer 1

Dictionary expects immutable object as a key, with a stable GetHashCode / Equals implementation. This means that after object is placed into dictionary, value returned by GetHashCode should not change, and any changes made to this object should not affect Equals method.

Although PhysicalAddress class was designed immutable, it still contains a few extension points, where its immutability is flawed.

First, it can be changed through input byte array, which is not copied but passed by reference, like this:

var data = new byte[] { 1,2,3 };
var mac = new PhysicalAddress(data);
data[0] = 0;

Second, PhysicalAddress is not a sealed class, and can be changed by derived implementation through overriding Constructor / GetHashCode / Equals methods. But this use case looks more like a hack, so we will ignore it, as well as modifications through reflection.

Your situation can only be achieved by first placing PhysicalAddress object into dictionary, and then modifying its source byte array, and then wrapping it into new PhysicalAddress instance.

Luckily, PhysicalAddress' GetHashCode implementation computes hash only once, and if same instance is modified, it is still placed into same dictionary bucket, and located again by Equals.

But, if source byte array is passed into another instance of PhysicalAddress, where hash was not yet computed - hash is recomputed for new byte[] value, new bucket is located, and duplicate is inserted into dictionary. In rare cases, same bucket can be located from new hash, and again, no duplicate is inserted.

Here's the code which reproduces the problem:

using System;
using System.Collections.Generic;
using System.Net.NetworkInformation;

class App
{
  static void Main()
  {
    var data = new byte[] { 1,2,3,4 };
    var mac1 = new PhysicalAddress(data);
    var mac2 = new PhysicalAddress(data);
    var dictionary = new Dictionary<PhysicalAddress,string>();
    dictionary[mac1] = "A";
    Console.WriteLine("Has mac1:" + dictionary.ContainsKey(mac1));
    //Console.WriteLine("Has mac2:" + dictionary.ContainsKey(mac2));
    data[0] = 0;
    Console.WriteLine("After modification");
    Console.WriteLine("Has mac1:" + dictionary.ContainsKey(mac1));
    Console.WriteLine("Has mac2:" + dictionary.ContainsKey(mac2));

    dictionary[mac2] = "B";
    foreach (var kvp in dictionary)
      Console.WriteLine(kvp.Key + "=" + kvp.Value);
  }
}

Note the commented line - if we will uncomment it, "ContainsKey" method will precompute hash for mac2, and it will be the same even after modification.

So my recommendation is to locate piece of code which generates PhysicalAddress instances, and create new byte array copy for each constructor call.