Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Could you please critisize the logger class below? Can it be used in a multi threaded web environment? If not how can I improve it? Is there anything wrong with locking in WriteToLog method or multithreading in FlushLog method?

public class Logger
{
    private static Logger instance;
    private static Queue<LogData> logQueue;
    private static string logDir = HttpContext.Current.Server.MapPath(ConfigurationManager.AppSettings["LogDirectory"]);
    private static string logFile = "log.txt";
    private static int maxLogAge = Int32.Parse(ConfigurationManager.AppSettings["LogMaxAge"]);
    private static int queueSize = Int32.Parse(ConfigurationManager.AppSettings["LogQueueSize"]);
    private static DateTime LastFlushed = DateTime.Now;

    private Logger() { }

    public static Logger Instance
    {
        get
        {
            if (instance == null)
            {
                instance = new Logger();
                logQueue = new Queue<LogData>();
            }
            return instance;
        }
    }

    public void WriteToLog(string message)
    {
        lock (logQueue)
        {
            LogData logEntry = new LogData(message);
            logQueue.Enqueue(logEntry);

            if (logQueue.Count >= queueSize || DoPeriodicFlush())
            {
                FlushLog();
            }
        }            
    }

    private bool DoPeriodicFlush()
    {
        TimeSpan logAge = DateTime.Now - LastFlushed;
        if (logAge.TotalSeconds >= maxLogAge)
        {
            LastFlushed = DateTime.Now;
            return true;
        }
        else
        {
            return false;
        }
    }

    private void FlushLog()
    {
        System.Threading.ThreadPool.QueueUserWorkItem(q => {
            while (logQueue.Count > 0)
            {
                LogData entry = logQueue.Dequeue();
                string logPath = logDir + entry.LogDate + "_" + logFile;

                using (System.IO.StreamWriter file = new System.IO.StreamWriter(logPath, true, System.Text.Encoding.UTF8))
                {
                    file.WriteLine(string.Format("{0}\t{1}", entry.LogTime, entry.Message));
                }
            }
        });
    }

    ~Logger()
    {
        FlushLog();
    }
}

public class LogData
{
    public string Message { get; set; }
    public string LogTime { get; set; }
    public string LogDate { get; set; }

    public LogData(string message)
    {
        Message = message;
        LogDate = DateTime.Now.ToString("yyyy-MM-dd");
        LogTime = DateTime.Now.ToString("HH:mm:ss.fff tt");
    }
}

Thanks in advance,

share|improve this question
7  
Don't re-invent the wheel; use log4net. –  SLaks Feb 27 '13 at 18:21
1  
This particular wheel has been invented multiple times. I suggest you take a look at one of the several options available. –  Brian Rasmussen Feb 27 '13 at 18:21
1  
With worries like these you should be using something like log4net where all of these issues have been worked out: logging.apache.org/log4net –  Paul Sasik Feb 27 '13 at 18:21
1  
Your code is rather non-thread-safe. –  SLaks Feb 27 '13 at 18:21
3  
This is one of those areas where there are so many available (free and otherwise) mature and proven logging frameworks that attempting to create a robust performant one from scratch is not a great use of one's time. –  Jesse C. Slicer Feb 27 '13 at 18:41

4 Answers 4

up vote 4 down vote accepted

This code is not thread safe. You synchronize (lock) while adding to the queue, but your code which removes from the code does not lock the queue, and will always run on a background thread, which is going to cause potential race conditions.

If you really must write your own logging, I would, at the least, look into using ConcurrentQueue<T> to avoid the need for locking on adding. BlockingCollection<T> would make this far simpler, as you could just have a thread call GetConsumingEnumerable() to process items as they're added.

That being said, logging is something that's been handled many times, and handled well. You'd be far better off using something like the new Semantic Logging Application Block (from P&P) or even log4net.

share|improve this answer
    
Thanks for your answer! I'm gonna take a look at your suggestions. I did not even know the classes you mentioned. –  anilca Feb 27 '13 at 19:06
1  
@anilca Great - that being said, for logging, using a pre-made solution is really just about always a superior option. –  Reed Copsey Feb 27 '13 at 19:09

Your singleton is not thread safe in first place. Please look at : Thread safe singleton implementation in C#

It is better to use Synchronized wrapper of the Queue. Queue is not thread safe.

Then i think lock in the WriteToLog redundant if you use thread safe Queue. Because each thread will safely enqueue log message it can not lead that if statement there to a wrong state.

However lock is definitely needed in FlushLog since the Queue is being enumerated there (thread safe Queue does not help in enumeration).

Finally there is an IO operation in a singleton, it is best to handle possible exceptions that is likely to come from an IO operation. You would not like to thrash your singleton easily.

share|improve this answer

Easy way to make your logger thread safe:

Change:

private static Logger instance;
private static Queue<LogData> logQueue;

public static Logger Instance
{
    get
    {
        if (instance == null)
        {
            instance = new Logger();
            logQueue = new Queue<LogData>();
        }
        return instance;
    }
}

to:

private static Logger instance = new Logger();
private static ConcurrentQueue<LogData> logQueue = new ConcurrentQueue<LogData>();

public static Logger Instance
{
    get
    {
        return instance;
    }
}

I would honestly just use log4net and extend it to support your use case.

share|improve this answer
    
After that change will I still need lock statements in my methods? –  anilca Feb 28 '13 at 7:48
    
Anilca, yes you have to lock. But not all of them. Read my explanation that is given as an answer. It is not thread-safe only by making construction thread-safe or using Thread-safe data structure. There are other concerns. Please read the answers above. It is explained by myself and by Reed Copsey so far. –  oalan Feb 28 '13 at 8:55
    
And what if my framework is older than 4.0? Is it possible to make my logger thread safe? –  anilca Feb 28 '13 at 10:03
1  
You can check what are the available data structures and thread api for the specific version of the framework that you use by checking documentation. Basically I worked with .NET before but currently (and mostly using Java) and regardless of the framework, platform there are some rules like you have to pick thread-safe data structure and you have to lock when you are iterating over that data structure. Therefore by taking into account those rules you can pick up the right api classes and methods for your specific case. –  oalan Feb 28 '13 at 10:17

Why hasn't anyone suggested Nlog? It's far better than Log4Net and is still actively maintained, unlike Log4Net, which hasn't been updated since version 1.2.10, published April 19, 2006. Also, Log4Net documentation blows.

http://stackoverflow.com/questions/710863/log4net-vs-nlog

share|improve this answer
    
Last log4net release was 2012-11-29 –  Romoku Feb 28 '13 at 13:02

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.