CODEWITHSUNDEEP
c#multithreadinglocking
Viktor Elofsson
Viktor Elofsson

Reputation: 1601

Multithreading, generic locks

I have a singleton class which looks a lot like this,

public class CfgHandler
{
    private static readonly string ConfigDir = "Config";

    public T Get<T>() where T : class, new()
    {
        string cfgFile = Path.Combine(ConfigDir, typeof(T).FullName + ".json");

        if (File.Exists(cfgFile))
        {
            var reader = new JsonReader();
            return reader.Read<T>(File.ReadAllText(cfgFile));
        }

        return null;
    }

    public void Set<T>(T instance) where T : class, new()
    {
        string cfgFile = Path.Combine(ConfigDir, typeof(T).FullName + ".json");

        var writer = new JsonWriter();
        string json = writer.Write(instance);

        File.WriteAllText(cfgFile, json);
    }
}

The class is used in a multithreaded environment and I want to add locks. But not one lock for the whole class, since I don't want a race condition between cfg.Set<Foo>(); and cfg.Set<Bar>() as they work with different data.

I've thought about adding the following class to CfgHandler,

private static class Locks<T>
{
    private static object _lock = new object();
    public static object Lock { get { return _lock; } }
}

and then lock like this (both for Get and Set),

public void Set<T>(T instance) where T : class, new()
{
    lock(Locks<T>.Lock)
    {
        // save to disk
    }
}

Am I missing something trivial? Is there a better way of achieving my goal?

Upvotes: 3

Views: 989

Answers (2)

Spence
Spence

Reputation: 29342

Please see my question here for more details: Are static members of generic classes shared between types

The implementation you have is simple but effective, it will prevent concurrent access to the Set<T>(T Instance) call correctly. My only advice is that the lock duration should be limited if you are making many concurrent calls to this API. For instance you could do all the work, but then only lock the call to the writer.write(instance) call, which is the only non-threadsafe work you appear to be doing in the call.

As an aside you have the potential to improve your code on the Get call, please see my answer here Is there a way to check if a file is in use? regarding your check for the file existing.

Upvotes: 2

sinelaw
sinelaw

Reputation: 16553

Lock per instance or lock per type?

The way you are doing it (with a static Locks<T>.Lock) means that every call to Set<Foo> even on a different instance of CfgHandler will share the same lock. Is that what you want? I'm guessing you may be better off just locking per instance - it will save you the complexity of Locks<T>. Just declare a private instance member (private object _lock = new object();) and use it (lock(this._lock))

EDIT If you're using a singleton instance of CfgHandler and want to lock per type, then I guess your approach is perfectly fine. If you're not using a single instance, but still want to lock per type then just make sure to use an instance of Locks<T> instead of making it static.

Upvotes: 5

Related Questions