What am I doing wrong in this multitheading example?

this is my first adventure into multi threading and I think I am missing some key concepts so any help will be appreciated. I am trying to create a log manager for an asp.net application. We will be logging views/inserts/modifies/deletes on a ton of data in our system. Instead of constantly inserting rows I thought that maybe if I created a singleton to hold a list of log entries in memory until it reached a certain size then write them all to the db at once. Then I thought maybe running this on a new thread will improve performance when the log needs to be written. Below is my test code. If I remove threading I get the 500 rows in the db that I expect but when I use multithreading I get around 200-300. Around half of the records aren't getting inserted. Is this a valid use for multithreading, and what am I doing wrong? Thank You.

LogManager:

  public sealed class LogManager
  {
    private static LogManager _Log = null;
    private static readonly object singletonLock = new object();
    private static readonly object listLock = new object();

    private List<LogEntry> LogEntries { get; set; }

    public static LogManager Log
    {
      get
      {
        if (_Log == null)
        {
          lock (singletonLock)
          {
            if (_Log == null)
            {
              _Log = new LogManager();
            }
          }
        }
        return _Log;
      }
    }

    public LogManager()
    {
      LogEntries = new List<LogEntry>();
    }

    public void Add(LogEntry logEntry)
    {
      lock (listLock)
      {
        LogEntries.Add(logEntry);
        if (LogEntries.Count >= 100)
        {          
          ThreadStart thread = delegate { Flush(new List<LogEntry>(LogEntries)); };
          new Thread(thread).Start();
          //Flush(LogEntries);          
          LogEntries.Clear();
        }
      }
    }

    private static void Flush(List<LogEntry> logEntries)
    {
      using (var conn = new SqlConnection(DAL.ConnectionString))
      {
        using (var cmd = conn.CreateCommand())
        {
          cmd.CommandType = CommandType.StoredProcedure;
          cmd.CommandText = "spInsertLog";
          conn.Open();
          foreach (var logEntry in logEntries)
          {
            cmd.Parameters.AddWithValue("@ID", logEntry.ID);
            try
            {
              cmd.ExecuteNonQuery();
            }
            catch (Exception ex) { throw (ex);/*KeepGoing*/}
            cmd.Parameters.Clear();
          }
        }      
      }
    }
  }

Console App:

  class Program
  {
    static void Main(string[] args)
    {
      var stopwatch = new Stopwatch();      
      f开发者_StackOverflowor (int i = 0; i < 500; i++)
      {
        stopwatch.Start();
        LogManager.Log.Add(new LogEntry() { ID = i });
        Console.WriteLine(String.Format("Count: {0}   Time: {1}",i.ToString(),stopwatch.ElapsedMilliseconds));
        stopwatch.Stop();
        stopwatch.Reset();
      }      
    }
  }

---

ThreadStart thread = delegate { Flush(new List<LogEntry>(LogEntries)); };
      new Thread(thread).Start();
      //Flush(LogEntries);          
      LogEntries.Clear();

List<LogEntry> is a reference type. Your new thread starts inserting them, but then you clear that list before it is finished. When you don't use multithreading, you wait for the entire list to be flushed before you clear it. You can fix this by changing the signature of Flush to take an array and doing

ThreadStart thread = delegate { Flush(LogEntries.ToArray()); };
      new Thread(thread).Start();
      //Flush(LogEntries);          
      LogEntries.Clear();

---

A couple of things that I see: First, I wouldn't use a List<> I would use a Queue<>, it's better suited for this situation. Secondly, right after you fire off the thread you clear the list. So there's a good chance that by the time the thread actually starts executing its code, the list is already empty. A Queue<> should help solve this problem, because you can remove items from the queue as they are written to the database.

Also, you should be locking your code while you're accessing the list, you could get an exception if an item is added to the list while you're iterating it. This would apply to a Queue<> as well, what I typically do is something like:

LogEntry myEntry;
lock(sync) {
   myEntry = myQueue.Dequeue();
}

Then also lock in your Add method (which you do).

---

Before I analyze a single line of your code, your intermediary storage for Log Messages is in the wrong place. I strongly recommend using MSMQ or some other queueing mechanism to store your messages while awaiting your LogManager for processing.

You are calling Flush in a separate thread and passing a reference to your list of log entries, then clearing the list in the current thread. You've effectively destroyed the list of entries the new thread was supposed to log. You need to pass a copy of the LogEntries list into the Flush thread before you clear your LogEntries field.

Perhaps something like:

{Flush(LogEntries.ToList())}

The LINQ expression ToList() will create a copy of the list for your Flush method.

As an aside, I would change your Flush method to take an IEnumerable<LogEntry> so that you can pass other collections, not just lists, into the method.