possible memory leak in a singleton?

Question

I've asked this question before with no real answer. Can anybody help? I'm profiling the below code inside a singleton and found that a lot of Rate objects (List<Rate>) are kept in memory although I clear them.

protected void FetchingRates()
{
  int count = 0;

  while (true)
  {
    try
    {
      if (m_RatesQueue.Count > 0)
      {
        List<RateLog> temp = null;

        lock (m_RatesQueue)
        {
          temp = new List<RateLog>();
          temp.AddRange(m_RatesQueue);
          m_RatesQueue.Clear();
        }

        foreach (RateLog item in temp)
        {
          m_ConnectionDataAccess.InsertRateLog(item);
        }

        temp.Clear();
        temp = null;
      }
      count++;
      Thread.Sleep(int.Parse(ConfigurationManager.AppSettings["RatesIntreval"].ToString()));
    }
    catch (Exception ex)
    {  
      Logger.Log(ex);                 
    }
  }
}

The insertion to the queue is made by:

public void InsertLogRecord(RateLog msg)
{
  try
  {
    if (m_RatesQueue != null)
    {
      //lock (((ICollection)m_queue).SyncRoot)
      lock (m_RatesQueue)
      {
        //insert new job to the line and release the thread to continue working.
        m_RatesQueue.Add(msg);
      }
    }
  }
  catch (Exception ex)
  {
    Logger.Log(ex);  
  }
}

The worker inserts rate log into DB as follows:

 internal int InsertRateLog(RateLog item)
    {
        try
        {
            SqlCommand dbc = GetStoredProcCommand("InsertRateMonitoring");
            if (dbc == null)
                return 0;
            dbc.Parameters.Add(new SqlParameter("@HostName", item.HostName));
            dbc.Parameters.Add(new SqlParameter("@RateType", item.RateType));
            dbc.Parameters.Add(new SqlParameter("@LastUpdated", item.LastUpdated));
            return ExecuteNonQuery(dbc);
        }
        catch (Exception ex)
        {
            Logger.Log(ex);
            return 0;
        }
    }

Any one sees a possible memory leak?

I don't know about memory leaks, but the way you're "handling" exceptions is extremely suspicious to start with. — Jon Skeet, Sep 10 '10 at 16:44
please notice- all execptions are logged to file system with a generic logger. the code lines for it were removed. — user437631, Sep 10 '10 at 16:47
It would be better to simply remove the try..catch blocks. Example code should be as simple as possible. — MikeWyatt, Sep 10 '10 at 16:51
How do you know there is a memory leak? Maybe the GC simply hasn't touched those objects yet. — MikeWyatt, Sep 10 '10 at 16:53
Your locking seems error-prone. Not all accesses to m_ratesQueue are protected by the same monitor. — Rauhotz, Sep 10 '10 at 16:54
.NET 4's `BlockingCollection` would make this code much nicer (if .NET 4) is an option (http://msdn.microsoft.com/en-us/library/dd267312.aspx) — Ian Mercer, Sep 10 '10 at 17:17

ChaosPandion · Answer 1 · 2010-09-10T17:25:04.993

I hope you are properly disposing the ADO.NET objects. (This is simply good practice.)
Any stray references will keep your RateLog objects from being collected by the GC.

I recommend you look through your code starting where the RateLog objects are being created and take note of all the places a reference is being held. Here are some things to consider.

Are the RateLog objects subscribed to any events?
Are you keeping a collection of RateLog objects sitting somewhere in a static class?

You should also consider encapsulating all your thread safety boilerplate in class.

public sealed class WorkQueue<T>
{
    private readonly System.Collections.Generic.Queue<T> _queue = new System.Collections.Generic.Queue<T>();
    private readonly object _lock = new object();

    public void Put(T item)
    {
        lock (_lock)
        {
            _queue.Enqueue(item);
        }
    }


    public bool TryGet(out T[] items)
    {
        if (_queue.Count > 0)
        {
            lock (_lock)
            {
                if (_queue.Count > 0)
                {
                    items = _queue.ToArray();
                    _queue.Clear();
                    return true;
                }
            }
        }

        items = null;
        return false;
    }
}

This will make your code a lot clearer:

protected void FetchingRates()
{
    int ratesInterval = int.Parse(ConfigurationManager.AppSettings["RatesIntreval"].ToString());
    int count = 0;
    var queue = new WorkQueue<RateLog>();

    while (true)
    {
        try
        {
            var items = default(RateLog[]);
            if (queue.TryGet(out items))
            {
                foreach (var item in items)
                {
                    m_ConnectionDataAccess.InsertRateLog(item);
                }
            }
        }
        catch (Exception ex)
        {  
            Logger.Log(ex);                 
        }

        Thread.Sleep(ratesInterval);
        count++;
    }
}

i'm using an one open SqlConnection. should i dispose my SqlComamnd objects?do they keep refrence to my rate logs? — user437631, Sep 10 '10 at 17:01
@user437631: Yes, you should. The general rule is: Everything that implements `IDisposable` also requires that you dispose it (or that at least someone does). Wrap your `SqlCommand` in a `using` statement: `using (SqlCommand dbc = GetStoredProcCommand("InsertRateMonitoring")) { /* do stuff */ }` — Dirk Vollmar, Sep 10 '10 at 17:07
@user437631 - As @0xA3 said you should dispose of the command objects. The command objects themselves won't keep a reference to the `RateLog` object but you may be keeping a reference somewhere else in your code. — ChaosPandion, Sep 10 '10 at 17:09
how can i force the rate log to the GC fter submitting it to the Database? — user437631, Sep 10 '10 at 17:17
@user437631 - It is bad practice to try and force the GC to handle your code. Without more information it would be pretty hard to give you definitive advice. — ChaosPandion, Sep 10 '10 at 17:28
what more information can i give? if i create my temp list of rates in the singleton- could this be the problem? — user437631, Sep 10 '10 at 17:42
@user437631 - First properly dispose of your ADO.NET objects to see if that resolves your issue. — ChaosPandion, Sep 10 '10 at 17:48

score 2 · Accepted Answer · answered Sep 10 '10 at 16:55

2

Looks like you are not disposing of your SqlCommand which is hanging onto a RateLog.

answered Sep 10 '10 at 16:55

tidwall

6,881
2
36
47

SwDevMan81 · Answer 3 · 2010-09-10T17:01:53.520

How about moving the temp creation outside the loop. You are probably not allowing the GC to clean up.

protected void FetchingRates()
{
  int count = 0;
  List<RateLog> temp = new List<RateLog>();

  while (true)
  {
    try
    {
      if (m_RatesQueue.Count > 0)
      {    
        lock (m_RatesQueue)
        {
          temp.AddRange(m_RatesQueue);
          m_RatesQueue.Clear();
        }

        foreach (RateLog item in temp)
        {
          m_ConnectionDataAccess.InsertRateLog(item);
        }

        temp.Clear();
      }
      count++;
      Thread.Sleep(int.Parse(ConfigurationManager.AppSettings["RatesIntreval"].ToString()));
    }
    catch (Exception ex)
    {                   
    }
  }
}

After temp.Clear() you might try adding GC.Collect();. This should NOT be the permanent solution, but could be used for your profiling to see if the objects get cleaned up eventually. If not, then there might still be a reference or event attached somewhere.

It actually should *not* matter whether the temp list is declared inside or outside the loop. — Dirk Vollmar, Sep 10 '10 at 17:01
Right, its just a bit cleaner. The *rest* of my answer addresses what the OP is seeing — SwDevMan81, Sep 10 '10 at 17:05

score 0 · Answer 4 · answered Sep 10 '10 at 16:50

0

the Clear() function deconstructs the List. But what about the RateLog instances? Is their deconstructor called? What about the lock, maybe this prevents that the RateLog from beeing deleted.

answered Sep 10 '10 at 16:50

sled

14,525
3
42
70

what should i do?make them Idisposable and loop them and call dispose? – user437631 Sep 10 '10 at 16:53
Depends on whats in the class... unmanaged objects? Objects that need to be disposed?, etc – SwDevMan81 Sep 10 '10 at 16:55
no. very simple class with three string\date time\int properties. – user437631 Sep 10 '10 at 17:02
In .NET there is no destructor that is explicitly called. There is a finalizer that is called by the garbage collector on objects that require finalization. It looks similar to a destructor, but is a different concept. – Dirk Vollmar Sep 10 '10 at 17:02

possible memory leak in a singleton?

4 Answers4