current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 4 down vote favorite
1

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;
        }
    }
}
share|improve this question
up vote 2 down vote accepted

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);
            }
        }
    }
}
share|improve this answer
    
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. – jeremywho Jul 2 '12 at 20:54
    
BeginInvoke already uses the thread pool behind the scenes. – Jesse C. Slicer Jul 2 '12 at 21:05
    
Maybe using Tasks is a better approah? – Arjang Jul 3 '12 at 0:49
    
That was going to be my first choice, but I need to use .net 3.5. – jeremywho Jul 3 '12 at 1:13
    
How to do this using tasks? codereview.stackexchange.com/questions/13263/… – Arjang Jul 3 '12 at 6:34
up vote 1 down vote

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.

share|improve this answer
    
I purposefully left error handling out for readability. – jeremywho Oct 18 '13 at 6:54
    
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. – jeremywho Oct 18 '13 at 7:01

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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