Acceptable to use a Type for a Dictionary Key?

Question

I would like to make a class that can store at most one copy of an object. All the objects stored here will share the same base class, and I would like to be able to get an object based on it's type.

I've come up with this solution so far, but I feel like I'm doing something wrong with using a Type for the Dictionary key.

Base class used in multiple modules

interface ISessionVariables { }

Example of common singleton class used for accessing

public class SessionVariables
{
    private object _sync = new object();
    private Dictionary<Type, ISessionVariables> _sessionVariables = 
        new Dictionary<Type, ISessionVariables>;

    public T Get<T>()
        where T : ISessionVariable, new()
    {
        lock (_sync)
        {
            ISessionVariables rtnValue = null;
            if (_sessionVariables.TryGetValue(typeof(T), out rtnValue))
                return (T)rtnValue;

            rtnValue = new T();
            _sessionVariables.Add(typeof(T), rtnValue);

            return (T)rtnValue;
        }
    }
}

This way I can call it like this from individual modules

SessionVariableSingleton.Get<ModuleASessionVars>().PropertyA;

SessionVariableSingleton.Get<ModuleCSessionVars>().PropertyC;

Is this an acceptable way of storing this kind of data structure? Or is there a better alternative using a List or a dictionary without a Type key?

Speedy

I don't see any problems with what you've decided to do. – Cinematograph

For two days now I have been writing code very much similar to yours. I haven't found any problems with it. Yet. If it makes you feel better (safer), just use the name of the Type instead. – Vardon

Looks good to me. Thinking of it as a dictionary might feel odd but when you see it as a hash map I think it feels like it fits. – Vaughnvaught

Since this is working fine, have you thought about asking over at Code Review? – Fishwife

For info, I edited the static cache implementation in my answer to be insanely simple, while offering full thread-safety. Sometimes, the simpler you make it, the better - this is one of those times. – Croquette

Answer 1

Yes Type is fine as a key; thread-safety is a concern, though - in many ways Hashtable is better at threaded-scenarios. However, there is a better option since you are using generics: cheat:

class SessionVariables {
    static class Cache<T> where T : ISessionVariable, new() {
        public static readonly ISessionVariable Value = new T();
    }
    ISessionVariable Get<T>() where T : ISessionVariable, new() {
        return Cache<T>.Value;
    }
}

Which is now fully thread-safe (without "returned different instances" issues) without any dictionary costs.

---

Edit on the topic of Hashtable for Jon:

Dictionary<TKey,TValue> makes no guarantees on concurrency, so you are required to synchronize all access - including the reads, as another thread doing a write can break a reader (you can force this in an example, but like most thread-races, it is hard to reproduce).

By contract, Hashtable guarantees that it is safe for any number of readers, plus at most one writer. From MSDN:

Hashtable is thread safe for use by multiple reader threads and a single writing thread. It is thread safe for multi-thread use when only one of the threads perform write (update) operations, which allows for lock-free reads provided that the writers are serialized to the Hashtable.

This means that you can do things like:

var val = (SomeType)hash[key];
if(val == null) {
   // not there; actually compute / create the value
   val = ...
   // and store it for the next access
   lock(syncLock) {
       hash[key] = val; // note: could do double-check here
   }
}
return val;

Notice that the read cycle above does not require any synchronization; only the writes need to be synchronized. Note also that because Hashtable uses object, it works best when the keys and values are classes (not structs).

Yes, concurrent dictionaries now exist - but the above works just fine.

Croquette

Not sure about your Hashtable comment - could you elaborate? I like the generic cache, although of course it only works properly when it is used as a singleton - creating a new instance of SessionVariables will give the same variables back. Also, I'm not sure why you haven't just restricted T in Cache<T> and used a static initializer - then it would always return the same instance with no safety issues. (Although in that case if the constructor failed, you wouldn't get to try again.) – Monosyllable

@Jon added more detail on Hashtable – Croquette

@Jon I fixed the false-singleton issue, btw; much simpler and better now – Croquette

Interesting - I didn't know that about Hashtable. I wonder what the difference is internally... – Monosyllable

@Jon we really need to create a new gold medal for any time you say "I didn't know that". We wouldn't need to award it very often, I suspect ;p – Croquette

Thanks, I like this implementation. It won't work with what I am doing now since I had planned on adding save/load functionality eventually, but I will definitely keep this in mind for the future. – Speedy

Answer 2

There's one issue: it's not thread-safe. Given that you appear to be using this via a singleton, I'd imagine you do need it to be thread-safe.

If you're using .NET 4, you'd be best off using ConcurrentDictionary. Otherwise, add locking.

The ConcurrentDictionary code would look like this:

internal class SessionVariables
{
    private readonly ConcurrentDictionary<Type, ISessionVariables> dictionary
        = new ConcurrentDictionary<Type, ISessionVariables>();

    internal ISessionVariable Get<T>() where T : ISessionVariable, new()
    {
        return dictionary.GetOrAdd(typeof(T), _ => new T());
    }
}

(I'd also avoid singletons where possible, but that's a different matter.)

EDIT: To specifically address using a type as a dictionary key: yes, that's fine. In fact, you're fine with reference equality, as Type objects are effectively canonically - there's only one Type object to represent a particular type in a particular AppDomain, no matter how you ask for it. (This may not be true in dynamically generated assemblies using CodeDOM... not sure. But it should be fine for the normal case.)

Monosyllable

Thanks, I'm using .Net 3.5 and my intention was to use a lock object in the Get<T> method. I just left it out of the question for simplicity. – Speedy

@Rachel: Well if you're asking whether there's anything wrong with the code, leaving out something which makes it safe is a bit odd... please give more complete information in future. – Monosyllable

@JonSkeet I will, thank you. I've also updated my post to include the locking and the .net 3.5 tag, although I will definitely keep ConcurrentDictionary in mind for if I'm ever using 4.0. I was mostly concerned about using a Type for a dictionary key, but it seems like that's perfectly acceptable. :) – Speedy