Async file writer in .Net 3.5

Question

I'm curious what people think about this little bit of logging code. It seems to work ok. Is there anything I'm missing?

public static class Logger
{
 private static readonly object Locker = new object();
 private static readonly StreamWriter Writer = 
 new StreamWriter("c:\\temp\\logtester.log", true) ;
 public static void Log(string message)
 {
 Action<string> action = MessageWriter;
 action.BeginInvoke(message, null, null);
 }
 private static void MessageWriter(string message)
 {
 lock(Locker)
 {
 Writer.WriteLine(message);
 }
 }
}

Using Jesse's code, but with QueueUserWorkItem instead of BeginInvoke.

public sealed class Logger : IDisposable
{
 private static readonly object Locker = new object();
 private static readonly StreamWriter Writer = new StreamWriter("c:\\temp\\logtester.log", true);
 private static bool _disposed;
 public static void Log(string message)
 {
 ThreadPool.QueueUserWorkItem(MessageWriter, message);
 }
 private static void MessageWriter(object message)
 {
 if (!(message is string)) return;
 lock (Locker)
 {
 Writer.WriteLine(message);
 }
 }
 ~Logger()
 {
 Dispose(false);
 }
 public void Dispose()
 {
 Dispose(true);
 GC.SuppressFinalize(this);
 }
 private void Dispose(bool disposing)
 {
 lock (Locker)
 {
 if (_disposed)
 return;
 if (disposing)
 {
 if (Writer != null)
 Writer.Dispose();
 }
 _disposed = true;
 }
 }
}

• Manos: When do you actually call writer.Dispose so the writer writes?

Answer 1

Here's a slightly different version. It implements IDisposable so that the StreamWriter can eventually be closed and disposed properly and also uses a typed delegate for asynchronous invocation and properly calls EndInvoke upon completion.

public sealed class Logger : IDisposable
{
 private delegate void WriteMessage(string message);
 private static readonly Logger Instance = new Logger("c:\\temp\\logtester.log");
 private readonly object Locker = new object();
 private readonly StreamWriter Writer;
 private bool Disposed;
 private Logger(string logFileName)
 {
 Writer = new StreamWriter(logFileName, true);
 }
 ~Logger()
 {
 Dispose(false);
 }
 public static void Log(string message)
 {
 WriteMessage action = Instance.MessageWriter;
 action.BeginInvoke(message, MessageWriteComplete, action);
 }
 public void Dispose()
 {
 Dispose(true);
 GC.SuppressFinalize(this);
 }
 private static void MessageWriteComplete(IAsyncResult iar)
 {
 ((WriteMessage)iar.AsyncState).EndInvoke(iar);
 }
 private void Dispose(bool disposing)
 {
 lock (Locker)
 {
 if (Disposed)
 {
 return;
 }
 if (disposing)
 {
 if (Writer != null)
 {
 Writer.Dispose();
 }
 }
 Disposed = true;
 }
 }
 private void MessageWriter(string message)
 {
 lock (Locker)
 {
 if (!Disposed && (Writer != null))
 {
 Writer.WriteLine(message);
 }
 }
 }
}

• jeremywho: Maybe using the threadpool would be more appropriate than using begininvoke? Since I don't care about a return value. I'll update the original question with this.
• Jesse C. Slicer: BeginInvoke already uses the thread pool behind the scenes.
• jimjim: Maybe using Tasks is a better approah?
• jeremywho: That was going to be my first choice, but I need to use .net 3.5.
• jimjim: How to do this using tasks? codereview.stackexchange.com/questions/13263/…

Answer 2

There is a lot of issues in this code from my point of view.

• No error handling.

Your writing operation could fail (insufficient space, security issues etc). In this case your log would bring down entire application.

If you'll have an error in the following line of code:

 private static readonly StreamWriter Writer = 
 new StreamWriter("c:\\temp\\logtester.log", true) ;

Your type would be marked as unsuable and client of your code will receive TypeLoadExpetion.

• Performance and responsiveness

Usually loggers are build using some sort of queue. I can suggest to implement something like Producer-Consumer queue; In this case you can write or flush all the data in more paritucar times, because currently we can exhaust thread pool threads by calling Write method too many times.

And as I mentioned at the comment it's not a good idea to close in Dispose method some static resources. In this case you can easily faced a situation when one instance would use already closed stream.

• jeremywho: I purposefully left error handling out for readability.
• jeremywho: I purposefully left error handling out for readability. You're correct about everything mentioned. I was mostly curious about feedback on using the TPL/threadpool in that manner. If I was implementing something now in .Net 4.5, I would probably use a BlockingCollection and do async file i/o operations.