Deadlock watchdog for shared structures

Question

In our application we have data which needs to be shared between multiple threads. Now and then we ran into a race condition and we could not figure out where that was. So I have implemented a deadlock watchdog.

It is supposed to be used like this (example):

public bool GetFromElementExistsCache(string id)
{
  using (new Lock(_syncElementExistsCache))
  {
    if (_elementExistsCache.ContainsKey(id))
    {
      return _elementExistsCache[id];
    }
    else
    {
      return false;
    }
  }
}

The implementation looks like this:

public sealed class Lock : IDisposable
{
  private static readonly object CACHE_LOCK = new object();
  private static readonly LogService LOG = new LogService(typeof(Lock));

  private readonly object _lockObject;
  private readonly bool _lockAcquired;

  private static readonly List<LockInformation> LOCK_OBJECTS = new List<LockInformation>();

  public Lock(object lockObject)
  {
    _lockObject = lockObject;
    _lockAcquired = false;

    UpdateLockInfo(LockInformation.LockRequest.Reqested);
    Monitor.Enter(_lockObject, ref _lockAcquired);

    if (!_lockAcquired)
    {
      LOG.Warn($"Could not get lock, possible deadlock detected for lock object {_lockObject.GetHashCode()}!");
    }
    UpdateLockInfo(LockInformation.LockRequest.Locked);
  }

  private void UpdateLockInfo(LockInformation.LockRequest lockStatus)
  {
    lock (CACHE_LOCK)
    {
      try
      {
        var lockExists = false;
        LockInformation lockInfo = null;
        foreach (var li in LOCK_OBJECTS)
        {
          if (li.LockObject == _lockObject && li.Thread == Thread.CurrentThread)
          {
            lockInfo = li;
          }
          if (li.LockStatus != LockInformation.LockRequest.Released)
          {
            lockExists = true;
          }
        }

        if (!lockExists)
        {
          LOCK_OBJECTS.Clear();
        }

        // the current thread is allowed to update the status if it has created the lock entry
        if (lockInfo != null)
        {
          if (lockStatus == LockInformation.LockRequest.Released)
          {
            LOCK_OBJECTS.Remove(lockInfo);
          }
          else if (lockInfo.LockStatus == LockInformation.LockRequest.Released || lockInfo.Thread == Thread.CurrentThread)
          {
            lockInfo.LockStatus = lockStatus;
            lockInfo.Thread = Thread.CurrentThread;
          }
        }
        else
        {
          LOCK_OBJECTS.Add(new LockInformation
          {
            LockStatus = lockStatus,
            LockObject = _lockObject,
            Thread = Thread.CurrentThread
          });
        }
      }
      catch (Exception e)
      {
        LOG.Warn("Error during updating lock information!", e);
      }
    }
  }

  public void Dispose()
  {
    if (_lockAcquired)
    {
      Monitor.Exit(_lockObject);
      UpdateLockInfo(LockInformation.LockRequest.Released);
    }
  }
}

The class which holds the lock information

internal class LockInformation
{
  public enum LockRequest
  {
    Reqested,
    Locked,
    Released
  } 

  public object LockObject { get; set; }
  public LockRequest LockStatus { get; set; }
  public Thread Thread { get; set; }

  public override bool Equals(object obj)
  {
    var lockInformation = obj as LockInformation;
    if (lockInformation != null)
    {
      return lockInformation.LockObject == LockObject;
    }
    return false;
  }

  protected bool Equals(LockInformation lockInfo)
  {
    if (lockInfo == null)
    {
      return false;
    }

    return lockInfo.LockObject == LockObject;
  }

  public override int GetHashCode()
  {
    unchecked
    {
      int hashCode = LockObject?.GetHashCode() ?? 0;
      hashCode = (hashCode * 397) ^ LockStatus.GetHashCode();
      hashCode = (hashCode * 397) ^ (Thread?.GetHashCode() ?? 0);
      return hashCode;
    }
  }

  public override string ToString()
  {
    var threadName = Thread.Name;
    if (string.IsNullOrEmpty(threadName))
    {
      threadName = "null";
    }
    return $"{LockObject.GetHashCode()} : {LockStatus} - {threadName}({Thread.ManagedThreadId})";
  }
}

There is an additional class which takes care of the deadlock detection.

So far the implementation works just fine. But I was wondering if you guys see any problemens in for example performance or thread identification etc. since I do not have much experience there.

Also general tips for coding style etc. are much appreciated as well!

Answer 1

I think this class creates more problems than it solves.

1. It locks on static object, which is a pretty bad idea. I mean, the obvious problem is that you end up synchronizing code blocks which use different lockObjects and would otherwise be unrelated. But what also bugs me is that it looks like a pretty major modification to your original use case, which might lead to wrong conclusions about the nature of original bug.

2. UpdateLockInfo method is pretty hard to follow. You do a lot of weird stuff. For example, why would you call LOCK_OBJECTS.Clear() if you already call LOCK_OBJECTS.Remove for released objects a few lines later? Or why would you set lockInfo.Thread = Thread.CurrentThread if you've already checked for equality? Those things make your code look fishy and bug-prone. Restructuring it might help. For example:

   var info = FindExistingLockInfo();
   if (info == null)
   {
       if (lockStatus != LockInformation.LockRequest.Requested) throw ...;
       LOCK_OBJECTS.Add(new ...);
   }
   else if (lockStatus == LockInformation.LockRequest.Released)
   {
       LOCK_OBJECTS.Remove(info);
   }
   else
   {
       info.LockStatus = lockStatus;
   }
   

3. Note, that neither the version above, nor your original implementation will work correctly for nested locks. To fix that you would have to implement some sort of reference counting.

4. Your have inconsistent equality comparison. When you look for existing lockInfo you check ThreadId, when you call LOCK_OBJECTS.Remove - you don't. This can lead to incorrect items being deleted.

5. It looks like your code is prone to leaking locks. For example, if Lock constructor throws after Monitor.Enter call due to denied access to log file, or due to thread being aborted, or due to any other reason, the lockObject is never released.

6. Personally, I don't quite see how !_lockAcquired is any indication of possible deadlock. But maybe I am just missing something about Monitor.Enter semantics.

I guess the bottom line is: if this class helps or have already helped you debug the problem you were having, then good for you, well done! :) However, in its current state I would not recommend using it in production code on regular basis. Multi-threading is already hard enough to get right, your class makes things even harder. Instead I would recommend following mr.eurotrash's advice. Refactor your code, so that you have only one access point to your shared resource, and then lock the entire thing from the inside. For us, mortals, in most cases this is the only bulletproof way to synchronize access.