C# Am i using lock correctly?

Question

I'm currently trying to write a thread-safe logger class. I'm not very familiar with correct design and best practices in this area. Is there a flaw in my code?

public class WriteStuff
{
 private readonly StreamWriter m_Writer;
 private readonly object m_WriteLock = new object ();

 public WriteStuff(String path)
 {
  m_Writer = File.CreateText (path);

  m_Writer.WriteLine ("x");
  m_Writer.Flush ();
 }

 public void ListenTo(Foo foo)
 {
  foo.SomeEvent += new EventHandler<SomeArgs> (Foo_Update);
 }

 private void Foo_Update(object sender, SomeArgs args)
 {
  lock (m_WriteLock) {
   m_Writer.WriteLine (args);
   m_Writer.Flush ();
  }
 }
}

Answer 1

Well, that looks OK to me; I'd probably implement IDisposable as a means to Close() the file, but...

Of course, you could also use any of the (many) pre-canned logging frameworks.

---

Update:

One thought: you might want to consider what happens if the file already exists; you don't want to stomp on your logs...

Answer 2

The event handler is on the same thread as the event generator which means your app could end up being held up by your log file write.

private void Foo_Update(object sender, SomeArgs args)        { 
    ThreadPool.QueueUserWorkItem(WriteAsync, args);
}

private void WriteAsync(object state) {  
    SomeArgs args = (SomeArgs)state;    
    lock (m_WriteLock) {                        
       m_Writer.WriteLine (args);                        
       m_Writer.Flush ();                
   }        
}

Answer 3

EDIT: this is plain nonsense from me, mistook the constructor for a method, not reading carefully enough. Sorry about that.

I'd advise to put the lock inside WriteStuff(), as otherwise unsynchronized access could occure.

Answer 4

What you've posted looks fine from a multi-threading perpective. Although I could be wrong, it would appear that any other code that does some multi-threading (even using the foo object) should be safe. Certainly, I can't see any deadlocks in the that section of code.

A few things worth noting anyway (apart from being very careful with deadlocks and testing rigourously to insure they won't occur):

• It's best to put a lock around the code within the constructor, as I believe it's possible in certain circumstances that methods can be called before the constructor block has finished executing. (Someone please correct me if I'm wrong on this one.)
• The StreamWriter object in this case is private, which is good. If it were protected or internal you would certainly have to be cautious about how other code utilised the object (in fact I think it would be best to almost always declare such objects as private).
• You've done locking the right way! It's always safest to lock on a separate private instance object because you know that object can't be locked by any other code than your own (which isn't the case if you lock this or the StreamWriter object itself).

Still, I may be missing something, and there is a small possibility that some other code not shown above might cause problems, but as far as I can see it that code isn't flawed except for a possible missing lock around the constructor code. You're more likely to have to watch out for deadlock situations when you start doing more complex multi-threading, especially across classes/instances.

Anyway, hope that helps.