Is this a valid use of the lock keyword in C#?

Question

I have a singleton class AppSetting in an ASP.NET app where I need to check a value and optionally update it. I know I need to use a locking mechanism to prevent multi-threading issues, but can someone verify that the following is a valid approach?

private static void ValidateEncryptionKey()
{
  if (AppSetting.Instance.EncryptionKey.Equals(Constants.ENCRYPTION_KEY, StringComparison.Ordinal))
  {
    lock (AppSetting.Instance)
    {
      if (AppSetting.Instance.EncryptionKey.Equals(Constants.ENCRYPTION_KEY, StringComparison.Ordinal))
      {
        AppSetting.Instance.EncryptionKey = GenerateNewEncryptionKey();
        AppSetting.Instance.Save();
      }
    }
  }
}

I have also seen examples where you lock on a private field in the current class, but I think the above approach is more intuitive.

Thanks!

Answer 1

Intuitive, maybe, but the reason those examples lock on a private field is to ensure that no other piece of code in the application can take the same lock in such a way as to deadlock the application, which is always good defensive practice.

If it's a small application and you're the only programmer working on it, you can probably get away with locking on a public field/property (which I presume AppSetting.Instance is?), but in any other circumstances, I'd strongly recommend that you go the private field route. It will save you a whole lot of debugging time in the future when someone else, or you in the future having forgotten the implementation details of this bit, take a lock on AppSetting.Instance somewhere distant in the code and everything starts crashing.

I'd also suggest you lose the outermost if. Taking a lock isn't free, sure, but it's a lot faster than doing a string comparison, especially since you need to do it a second time inside the lock anyway.

So, something like:

private object _instanceLock = new object () ;

private static void ValidateEncryptionKey()
{
  lock (AppSetting.Instance._instanceLock)
  {
    if (AppSetting.Instance.EncryptionKey.Equals(Constants.ENCRYPTION_KEY, StringComparison.Ordinal))
    {
      AppSetting.Instance.EncryptionKey = GenerateNewEncryptionKey();
      AppSetting.Instance.Save();
    }
  }
}

An additional refinement, depending on what your requirements are to keep the EncryptionKey consistent with the rest of the state in AppSetting.Instance, would be to use a separate private lock object for the EncryptionKey and any related fields, rather than locking the entire instance every time.