4
\$\begingroup\$

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;
        }
    }
}
\$\endgroup\$
1
  • \$\begingroup\$ When do you actually call writer.Dispose so the writer writes? \$\endgroup\$
    – Manos
    Nov 16, 2016 at 11:56

2 Answers 2

2
\$\begingroup\$

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);
            }
        }
    }
}
\$\endgroup\$
9
  • \$\begingroup\$ 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. \$\endgroup\$
    – jeremywho
    Jul 2, 2012 at 20:54
  • \$\begingroup\$ BeginInvoke already uses the thread pool behind the scenes. \$\endgroup\$ Jul 2, 2012 at 21:05
  • \$\begingroup\$ Maybe using Tasks is a better approah? \$\endgroup\$
    – jimjim
    Jul 3, 2012 at 0:49
  • \$\begingroup\$ That was going to be my first choice, but I need to use .net 3.5. \$\endgroup\$
    – jeremywho
    Jul 3, 2012 at 1:13
  • \$\begingroup\$ How to do this using tasks? codereview.stackexchange.com/questions/13263/… \$\endgroup\$
    – jimjim
    Jul 3, 2012 at 6:34
1
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ I purposefully left error handling out for readability. \$\endgroup\$
    – jeremywho
    Oct 18, 2013 at 6:54
  • \$\begingroup\$ 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. \$\endgroup\$
    – jeremywho
    Oct 18, 2013 at 7:01

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.