Where and How to apply lock

I'm trying to figure out the correct way of applying locking to the following class. In a nutshell, the object is a singleton and when created, builds a variable number of menus from xml files in a given directory. Right now, only reads are allowed thus no locking takes places (MSDN states reads are thread-safe from dictionary). But, I also have a file system watcher wired up so I can re-build menus when a change takes place. There are two dictionaries where reads occur and so I need a way to handle this. I could use Lock(this), but is there a better way? So, the only time I want to freeze reads is when the updating takes place (look in ctor).

Here is the class for a visual:

public class XmlMenuProvider : IMenuProvider {
    private readonly INavigationService navigation;
    private readonly Dictionary<string, IEnumerable<MenuItem>> menus;
    private readonly Dictionary<string, Dictionary<string, MenuItem>> menusLookup;
    private readonly FileSystemWatcher monitor;

    public XmlMenuProvider(string folderPath, INavigationService navigation)
    {
        this.navigation = navigation;
        this.menusLookup = new Dictionary<string, Dictionary<string, MenuItem>>();
        this.menus = LoadFromSourceDirectory(folderPath);
        this.monitor.Changed += (o, e) => {
                // TODO - Add Locking
    开发者_StackOverflow        };
    }

    public IEnumerable<MenuItem> GetMenuItems(string name) {
        return menus[name];
    }

    public MenuItem FindItemByName(string menu, string name) {
        return menusLookup[menu][name];
    }

    private Dictionary<string, IEnumerable<MenuItem>> LoadFromSourceDirectory(string folderPath) {
        var menus = new Dictionary<string, IEnumerable<MenuItem>>();
        foreach (var file in Directory.GetFiles(folderPath, "*.xml")) {
            var root = XDocument.Load(file).Elements().First();
            var name = root.Attribute("name").Value;

            var lookup = new Dictionary<string, MenuItem>();
            menusLookup.Add(name, lookup);
            menus.Add(name, BuildMenuHiearchyFromElement(root, lookup, null));
        }
        return menus;
    }

    private IEnumerable<MenuItem> BuildMenuHiearchyFromElement(XElement element, Dictionary<string, MenuItem> lookup, MenuItem parent) {
        return element.Elements("Item")
                      .Select(e => {
                          var mi = CreateMenuItemFromElement(e, lookup, parent);
                          lookup.Add(mi.Name, mi);
                          return mi;
                      }
                ).ToList();
    }

    private MenuItem CreateMenuItemFromElement(XElement element, Dictionary<string, MenuItem> lookup, MenuItem parent) {
        var name = element.Attribute("Name").Value;
        var display = element.Attribute("DisplayName").Value;
        var isClickable = true;

        var roles = element.Attribute("Roles").Value.Split(',');
        if (roles.Length == 1 && roles.First() == string.Empty) {
            roles = new string[] { };
        }
        var attrClick = element.Attribute("IsClickable");
        if (attrClick != null) {
            isClickable = bool.Parse(attrClick.Value);
        }
        var navigateUrl = string.Empty;
        if (isClickable) {
            navigateUrl = navigation.FetchDestination(name);
        }

        return new MenuItem(name, display, navigateUrl, isClickable, roles, x => BuildMenuHiearchyFromElement(element, lookup, x), parent);
    }
}

Thank You.

You should never use lock(this) but create a lockObject instead ( an istance of object private into the class ) and lock on that object instead of this.

There is a good write up of Singleton implementations and optimisations here (are you sure that the menu is a singleton - is it the same menu for all users?)

Since you want to optimise with locking only on the writes, you might also want to look at ReaderWriterLockSlim

Rather than lock, I think you might want to use a mutex that the updater routine claims when it begins and releases when it's finished.

It's commonly recommended to create a private object that you use for locking:

private object _sync = new Object();

By using a private object as identifier, only the code in the class can access it, so there is no risk that any code outside the class could lock using the same identifier and cause a deadlock.

If you use lock in the code that changes the data, you also need to use a lock in all code that reads the data, otherwise the lock is useless.

Note that the lock doesn't protect the data in any way, it's only a way to make sure that only one thread at a time enters a section of the code.