Multithreading, generic locks

Question

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?

Under what conditions do you want two calls to be allowed to be concurrent and when do you want them to be atomic? If your set is setting different types it doesn't look like it would cause problems, so why lock the entire set method (unless it's just easier than finding out when you really need to lock is, which is a perfectly acceptable answer). — Servy, Apr 26 '12 at 13:32

score 5 · Accepted Answer · answered Apr 26 '12 at 13:31

5

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.

answered Apr 26 '12 at 13:31

sinelaw

16,205
3
49
80

I'm trying to accomplish the first part of your answer, two/three `Set`s on different threads should share the same lock, but not the same as `Set`. – Viktor Elofsson Apr 26 '12 at 13:34
@vktr, what first part - do you have many, or only one, instance of CfgHandler? – sinelaw Apr 26 '12 at 13:38
CfgHandler is a singleton object although it doesn't show in the example I gave. – Viktor Elofsson Apr 26 '12 at 13:39
1

Ok, then what you are doing seems fine, except that I wouldn't be using a static `Locks` - in case you ever want to have more than a single instance of `CfgHandler`, you can have your `CfgHandler` hold an instance of `Locks` (and you'll also have to make sure the config filename is different for each instance of `CfgHandler`) – sinelaw Apr 26 '12 at 13:42

score 2 · Answer 2 · edited May 23 '17 at 12:12

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.

Multithreading, generic locks

2 Answers2