The Problem
I was debugging some Smart Client code today, wondering why the local cache of a table was returning empty rows after the client reconnected with the server. Besides stumbling into the obvious bug (the cache wasn't supposed to be updated), I also fixed the source of the first bug.
The code I was executing was:
Dictionary<string, string> parms = new Dictionary<string, string>();
parms["@UID"] = uid;
DataView dv = api.LoadViewIntoContainer("PhysicalKiosk", "ComputerName=@UID", parms);
parms["@UID"] = physicalKioskId;
DataView dv2 = api.LoadViewIntoContainer("Kiosk", "PhysicalKioskId=@UID", parms);
Now, because these views are set up as cached (so the client can load the data without the server being connected), my client API dutifully saves all the information to reconstruct this view later on, including the parameter list, which is stored in a ViewInfo
class. Here's the constructor, reduced to the essential issue:
public ViewInfo(Dictionary<string, string> parms)
{
this.parms = parms;
}
So, do you see the problem?
When I load the second DataView
, I'm reusing the parms
dictionary, so the ViewInfo
's parms
variable associated with the first DataView
gets updated with the new value! This is because the ViewInfo
is storing a reference to the parms
dictionary, so when the dictionary changes, all references to that dictionary see that change.
The Solution
The obvious solution was, create a new parms
instance so that the ViewInfo
instance gets a unique instance of the dictionary. Thinking about this, I realized I'm going to forget to do this in many other places as well, so why not do a bit of defensive programming and, in the ViewInfo
constructor, clone the dictionary. After a brief search, I didn't find any sample code that clones a dictionary, so I wrote one. The following code implements two types of cloning:
- if the key and value are value types or strings, the dictionary is copied "by hand"
- if the key or value are not value types, then the dictionary is serialized to a memory stream, then deserialized, effectively performing a deep copy
The code illustrates this:
public static Dictionary<K, V> CloneDictionary<K, V>(Dictionary<K, V> dict)
{
Dictionary<K, V> newDict=null;
if (dict != null)
{
if (((typeof(K).IsValueType || typeof(K) == typeof(string)) &&
(typeof(V).IsValueType) || typeof(V) == typeof(string)))
{
newDict = new Dictionary<K, V>();
foreach (KeyValuePair<K, V> kvp in dict)
{
newDict[kvp.Key] = kvp.Value;
}
}
else
{
BinaryFormatter bf = new BinaryFormatter();
MemoryStream ms = new MemoryStream();
bf.Serialize(ms, dict);
ms.Position = 0;
newDict = (Dictionary<K, V>)bf.Deserialize(ms);
}
}
return newDict;
}
You'll also note that the clone routine handles the case where the dict
parameter is null
. I figure it ought to--cloning a null should return a null. Also note that this is a good example of a generic method as compared to a generic class.
The fixed constructor now looks like this (as a good usage case):
public ViewInfo(Dictionary<string, string> parms)
{
this.parms = Clone.CloneDictionary<string, string>(parms);
}
Conclusion
It's rather embarrassing to have encountered this bug in the core code of my API, but it clearly illustrates how important it is to be conscious of saving references to collections and how easy it is to get into trouble, often much later, and by a simple quirk of trying to be smart and reusing something like a collection that is stored by some other part of the code.