0

I'm looking through some very old code trying to work out the cause of a long term problem causing from what I can best tell is a memory leak on the server.

I can't seem to wrap my head round what's going on in this method, but suspect the GC.SurpressFinalise() might be causing issues not releasing some of the memory. Could this be the case?

public class DistributedLock : IDisposable
{
    private IRedLock Lock { get; }
    private bool Disposed { get; set; }
    public bool Acquired => Lock.IsAcquired;

    public DistributedLock(string lockKey, TimeSpan expiry)
    {
        Disposed = false;

        var conn = RedLockController.GetConnection();
        Lock = conn.CreateLock(lockKey, expiry);
    }

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    ~DistributedLock()
    {
        Dispose(false);
    }

    protected virtual void Dispose(bool disposing)
    {
        if (Disposed) return;
        if (disposing)
        {
            Lock.Dispose();
        }
        Disposed = true;
    }
}
Tom Gullen
  • 61,249
  • 84
  • 283
  • 456
  • You either let the FInalizer run, or Dispose of the instance - never both. But as both codes are pretty similar (the only difference is if you relay it to cointained classes like Lock), usually it is both written in the Dipose function. So on calling Dispose, you indeed supress Finalisation. – Christopher Oct 21 '19 at 10:13
  • I see nothing wrong with the implementation of this Dispose Pattern. Indeed it seems to be literally the exampel code: https://learn.microsoft.com/en-us/dotnet/standard/garbage-collection/implementing-dispose – Christopher Oct 21 '19 at 10:15
  • 2
    As for the part of the "memory leak", are you certain there is one? Or did you just mistake the GC being lazy at collecting and thus the memory footprint increasing for a "memory leak"? It is a very common mistake. – Christopher Oct 21 '19 at 10:15
  • 1
    This code fully implements the IDisposable pattern, there is nothing wrong with that. It's a bit of an overkill and a performance hit because the class does not have unmanaged resources and so could [avoid declaring the finalizer](https://stackoverflow.com/a/898867/11683), but the `SuppressFinalize` call is correct. – GSerg Oct 21 '19 at 10:17
  • @Christopher we're having performance issues on the site where every now and then one web server will spike to writing 3-8GB of data on the disk and then lock up failing to connect to Redis. We're in Azure, and for the life of us can't figure out what's causing this. Best guess so far is that's paging memory and it's running out of memory at some point as there's nothing we can see being physically written to the disks. – Tom Gullen Oct 21 '19 at 10:18
  • The GC has one huge issue - while the collction is running, all other threads have to be stopped. As a result it tries to run as little as possible. Ideally it only runs once - during applciation closuer - where it would also have to do the least work. Before you run into a OOM Exception, it will have done all collection. But till then the Memory footprint can increase. And of coruse the OS might swap out the memory. – Christopher Oct 21 '19 at 11:32
  • There are actually different GC modes. The defautl is "Desktop", where it behaves like I said. But Server for example will do the tagging of unreachable instances in seperate threads, regulary do collection - but often on a timelimit - and skipping the memory defragmentation part of collection. Maybe the wrong GC mode is set? – Christopher Oct 21 '19 at 11:33

0 Answers0