38

I am using Enterprise Library 4 on one of my projects for logging (and other purposes). I've noticed that there is some cost to the logging that I am doing that I can mitigate by doing the logging on a separate thread.

The way I am doing this now is that I create a LogEntry object and then I call BeginInvoke on a delegate that calls Logger.Write.

new Action<LogEntry>(Logger.Write).BeginInvoke(le, null, null);

What I'd really like to do is add the log message to a queue and then have a single thread pulling LogEntry instances off the queue and performing the log operation. The benefit of this would be that logging is not interfering with the executing operation and not every logging operation results in a job getting thrown on the thread pool.

How can I create a shared queue that supports many writers and one reader in a thread safe way? Some examples of a queue implementation that is designed to support many writers (without causing synchronization/blocking) and a single reader would be really appreciated.

Recommendation regarding alternative approaches would also be appreciated, I am not interested in changing logging frameworks though.

Eric Schoonover
  • 47,184
  • 49
  • 157
  • 202
  • @spoon, I added a slightly improved version, keep in mind you will have to test it a lot before using it, cause I just knocked it up. – Sam Saffron Jul 27 '09 at 23:28
  • 1
    Although not applicable directly to this question (since it's talking EntLib 4), Enterprise Library 6 now supports Asynchronous Logging out of the box. – Randy Levy Aug 29 '13 at 12:20

10 Answers10

38

I wrote this code a while back, feel free to use it.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading;

namespace MediaBrowser.Library.Logging {
    public abstract class ThreadedLogger : LoggerBase {

        Queue<Action> queue = new Queue<Action>();
        AutoResetEvent hasNewItems = new AutoResetEvent(false);
        volatile bool waiting = false;

        public ThreadedLogger() : base() {
            Thread loggingThread = new Thread(new ThreadStart(ProcessQueue));
            loggingThread.IsBackground = true;
            loggingThread.Start();
        }


        void ProcessQueue() {
            while (true) {
                waiting = true;
                hasNewItems.WaitOne(10000,true);
                waiting = false;

                Queue<Action> queueCopy;
                lock (queue) {
                    queueCopy = new Queue<Action>(queue);
                    queue.Clear();
                }

                foreach (var log in queueCopy) {
                    log();
                }
            }
        }

        public override void LogMessage(LogRow row) {
            lock (queue) {
                queue.Enqueue(() => AsyncLogMessage(row));
            }
            hasNewItems.Set();
        }

        protected abstract void AsyncLogMessage(LogRow row);


        public override void Flush() {
            while (!waiting) {
                Thread.Sleep(1);
            }
        }
    }
}

Some advantages:

  • It keeps the background logger alive, so it does not need to spin up and spin down threads.
  • It uses a single thread to service the queue, which means there will never be a situation where 100 threads are servicing the queue.
  • It copies the queues to ensure the queue is not blocked while the log operation is performed
  • It uses an AutoResetEvent to ensure the bg thread is in a wait state
  • It is, IMHO, very easy to follow

Here is a slightly improved version, keep in mind I performed very little testing on it, but it does address a few minor issues.

public abstract class ThreadedLogger : IDisposable {

    Queue<Action> queue = new Queue<Action>();
    ManualResetEvent hasNewItems = new ManualResetEvent(false);
    ManualResetEvent terminate = new ManualResetEvent(false);
    ManualResetEvent waiting = new ManualResetEvent(false);

    Thread loggingThread; 

    public ThreadedLogger() {
        loggingThread = new Thread(new ThreadStart(ProcessQueue));
        loggingThread.IsBackground = true;
        // this is performed from a bg thread, to ensure the queue is serviced from a single thread
        loggingThread.Start();
    }


    void ProcessQueue() {
        while (true) {
            waiting.Set();
            int i = ManualResetEvent.WaitAny(new WaitHandle[] { hasNewItems, terminate });
            // terminate was signaled 
            if (i == 1) return; 
            hasNewItems.Reset();
            waiting.Reset();

            Queue<Action> queueCopy;
            lock (queue) {
                queueCopy = new Queue<Action>(queue);
                queue.Clear();
            }

            foreach (var log in queueCopy) {
                log();
            }    
        }
    }

    public void LogMessage(LogRow row) {
        lock (queue) {
            queue.Enqueue(() => AsyncLogMessage(row));
        }
        hasNewItems.Set();
    }

    protected abstract void AsyncLogMessage(LogRow row);


    public void Flush() {
        waiting.WaitOne();
    }


    public void Dispose() {
        terminate.Set();
        loggingThread.Join();
    }
}

Advantages over the original:

  • It's disposable, so you can get rid of the async logger
  • The flush semantics are improved
  • It will respond slightly better to a burst followed by silence
Sam Saffron
  • 128,308
  • 78
  • 326
  • 506
  • 2
    Your "waiting" flag should be volatile if it's being used from multiple threads - or use the lock when accessing it. I'm not sure I see the point in waiting for 10 seconds rather than just waiting without a timeout. I'd also call Set while you still hold the lock - it won't actually make much difference, but you could get two threads both adding events, then one thread setting the event, the reader completing the wait and copying the data, *then* the other writer setting the event again. No great loss, admittedly. Why do you have the loggingThread instance variable? – Jon Skeet Jul 27 '09 at 12:37
  • 2
    The 10 second wait, is to avoid a starvation, in the case where you get a burst of events that is partially serviced. (getting events just after the queue is copied) I should probably change this to a manual reset event, and perhaps add some logic to re-service the queue until its really empty, before waiting again. The thread var is just a tiny bit of future proofing that isn't really needed (in case I want to get the id of the thread or something). – Sam Saffron Jul 27 '09 at 12:58
  • Is there really a benefit by using a Thread object instead of spinning up a BackgroundWorker over and over? Isn't the BackgroundWorker just throwing jobs on the ThreadPool... which should manage the thread sharing/creation for you. – Eric Schoonover Jul 27 '09 at 16:30
  • 1
    @spoon, yerp there is an advantage, by using a single thread your more elegantly serialize the access, in a thread pool scenario you could end up having 100 threads servicing the queue which is not desirable – Sam Saffron Jul 27 '09 at 22:07
  • If you have only one BackgroundWorker running at any one time though you never actually have more than one thread working on the queue and you don't have to explicitly manage the thread associated with the BackgroundWorker. Right? – Eric Schoonover Jul 28 '09 at 00:14
  • 3
    true, but you will have to manage the lifetime of a single background worker, its really not the tool for the job cause you have no need for the progress facilities etc that come with it. – Sam Saffron Jul 28 '09 at 01:09
  • 1
    Before the hasNewItems.Reset(); it might be a good idea to put in a small delay, like 10 milleconds to allow the queue to build up some data. – FlappySocks Jul 31 '09 at 11:45
  • May I ask why you call `waiting.Set();` in `ProcessQueue()` ? Which thread is blocked so you call "set"? – Royi Namir Nov 05 '15 at 14:33
  • Very minor comments: - queueCopy can just be an array. No need for queue structure. I don't know if there's any actual overhead to a queue vs an array. - Personally I'd mark all five of those fields as readonly. - What happens if AsyncLogMessage throws an exception? Maybe put a try / catch block around log()? I might just make catch write the exception to console, since it's obviously not safe to log the exception at that point... – Kevin Holt Aug 16 '17 at 19:15
  • @RoyiNamir - it's used by the Flush call, to block whatever thread calls Flush until the queue has been processed. – Kevin Holt Aug 16 '17 at 19:16
  • There's an issue with Flush() (for version #2 above). If new actions are queued before the foreach in ProcessQueue has finished, then Flush only actually flushes out the actions in queueCopy, not anything that's been since added to the queue. I suggest this: `Flush() { bool finished=false; queue.Enqueue(() => finished=true); while (!finished) waiting.waitOne(); }` – Kevin Holt Aug 16 '17 at 19:22
14

Yes, you need a producer/consumer queue. I have one example of this in my threading tutorial - if you look my "deadlocks / monitor methods" page you'll find the code in the second half.

There are plenty of other examples online, of course - and .NET 4.0 will ship with one in the framework too (rather more fully featured than mine!). In .NET 4.0 you'd probably wrap a ConcurrentQueue<T> in a BlockingCollection<T>.

The version on that page is non-generic (it was written a long time ago) but you'd probably want to make it generic - it would be trivial to do.

You would call Produce from each "normal" thread, and Consume from one thread, just looping round and logging whatever it consumes. It's probably easiest just to make the consumer thread a background thread, so you don't need to worry about "stopping" the queue when your app exits. That does mean there's a remote possibility of missing the final log entry though (if it's half way through writing it when the app exits) - or even more if you're producing faster than it can consume/log.

Jon Skeet
  • 1,421,763
  • 867
  • 9,128
  • 9,194
  • Can you link to the .NET 4 documentation describing this? – Eric Schoonover Jul 25 '09 at 08:27
  • What is the difference between locking an object instance in the case of your ProducerConsumer example and just locking the Queue object? I'm just reading the test of your article now so I may just have not reached the explanation. – Eric Schoonover Jul 25 '09 at 08:48
  • 4
    I prefer to lock on separate objects which I absolutely *know* nothing else is going to lock on. I don't know whether Queue locks on itself internally... in this case it wouldn't matter if it did, but as a general principle, I like to keep locks "private" (so nothing else can interfere) unless I have a good reason to do otherwise. – Jon Skeet Jul 25 '09 at 09:09
  • @Jon, I just did a second implementation which supports dispose and uses a manual reset event. Would love to get the Skeet stamp of approval :p – Sam Saffron Jul 27 '09 at 23:30
3

Here is what I came up with... also see Sam Saffron's answer. This answer is community wiki in case there are any problems that people see in the code and want to update.

/// <summary>
/// A singleton queue that manages writing log entries to the different logging sources (Enterprise Library Logging) off the executing thread.
/// This queue ensures that log entries are written in the order that they were executed and that logging is only utilizing one thread (backgroundworker) at any given time.
/// </summary>
public class AsyncLoggerQueue
{
    //create singleton instance of logger queue
    public static AsyncLoggerQueue Current = new AsyncLoggerQueue();

    private static readonly object logEntryQueueLock = new object();

    private Queue<LogEntry> _LogEntryQueue = new Queue<LogEntry>();
    private BackgroundWorker _Logger = new BackgroundWorker();

    private AsyncLoggerQueue()
    {
        //configure background worker
        _Logger.WorkerSupportsCancellation = false;
        _Logger.DoWork += new DoWorkEventHandler(_Logger_DoWork);
    }

    public void Enqueue(LogEntry le)
    {
        //lock during write
        lock (logEntryQueueLock)
        {
            _LogEntryQueue.Enqueue(le);

            //while locked check to see if the BW is running, if not start it
            if (!_Logger.IsBusy)
                _Logger.RunWorkerAsync();
        }
    }

    private void _Logger_DoWork(object sender, DoWorkEventArgs e)
    {
        while (true)
        {
            LogEntry le = null;

            bool skipEmptyCheck = false;
            lock (logEntryQueueLock)
            {
                if (_LogEntryQueue.Count <= 0) //if queue is empty than BW is done
                    return;
                else if (_LogEntryQueue.Count > 1) //if greater than 1 we can skip checking to see if anything has been enqueued during the logging operation
                    skipEmptyCheck = true;

                //dequeue the LogEntry that will be written to the log
                le = _LogEntryQueue.Dequeue();
            }

            //pass LogEntry to Enterprise Library
            Logger.Write(le);

            if (skipEmptyCheck) //if LogEntryQueue.Count was > 1 before we wrote the last LogEntry we know to continue without double checking
            {
                lock (logEntryQueueLock)
                {
                    if (_LogEntryQueue.Count <= 0) //if queue is still empty than BW is done
                        return;
                }
            }
        }
    }
}
Eric Schoonover
  • 47,184
  • 49
  • 157
  • 202
  • It seems like you intend to make the AsyncLogger class a singleton. I'd make the constructor private in that case, but otherwise it seems that it gets the job done. – Abhijeet Patel Jul 26 '09 at 02:55
2

I suggest to start with measuring actual performance impact of logging on the overall system (i.e. by running profiler) and optionally switching to something faster like log4net (I've personally migrated to it from EntLib logging a long time ago).

If this does not work, you can try using this simple method from .NET Framework:

ThreadPool.QueueUserWorkItem

Queues a method for execution. The method executes when a thread pool thread becomes available.

MSDN Details

If this does not work either then you can resort to something like John Skeet has offered and actually code the async logging framework yourself.

Rinat Abdullin
  • 23,036
  • 8
  • 57
  • 80
  • 1
    I've already identified that there is a benefit to async logging. Also I am not in a position to switch logging frameworks as we use EntLib logging extensively. It would not be trivial or a welcome breaking change by the rest of the team :). ThreadPool.QueueUserWorkItem is basically what the BeginInvoke call that I am using now does. I think John's answer is the most appropriate so far for my situation. – Eric Schoonover Jul 25 '09 at 08:44
  • 1
    Quueing a work item does not guarantee any ordering so if you care about the order in which the messages are logged then that would not be a viable approach IMO. – Abhijeet Patel Jul 25 '09 at 17:14
1

In response to Sam Safrons post, I wanted to call flush and make sure everything was really finished writting. In my case, I am writing to a database in the queue thread and all my log events were getting queued up but sometimes the application stopped before everything was finished writing which is not acceptable in my situation. I changed several chunks of your code but the main thing I wanted to share was the flush:

public static void FlushLogs()
        {   
            bool queueHasValues = true;
            while (queueHasValues)
            {
                //wait for the current iteration to complete
                m_waitingThreadEvent.WaitOne();

                lock (m_loggerQueueSync)
                {
                    queueHasValues = m_loggerQueue.Count > 0;
                }
            }

            //force MEL to flush all its listeners
            foreach (MEL.LogSource logSource in MEL.Logger.Writer.TraceSources.Values)
            {                
                foreach (TraceListener listener in logSource.Listeners)
                {
                    listener.Flush();
                }
            }
        }

I hope that saves someone some frustration. It is especially apparent in parallel processes logging lots of data.

Thanks for sharing your solution, it set me into a good direction!

--Johnny S

1

I wanted to say that my previous post was kind of useless. You can simply set AutoFlush to true and you will not have to loop through all the listeners. However, I still had crazy problem with parallel threads trying to flush the logger. I had to create another boolean that was set to true during the copying of the queue and executing the LogEntry writes and then in the flush routine I had to check that boolean to make sure something was not already in the queue and the nothing was getting processed before returning.

Now multiple threads in parallel can hit this thing and when I call flush I know it is really flushed.

     public static void FlushLogs()
    {
        int queueCount;
        bool isProcessingLogs;
        while (true)
        {
            //wait for the current iteration to complete
            m_waitingThreadEvent.WaitOne();

            //check to see if we are currently processing logs
            lock (m_isProcessingLogsSync)
            {
                isProcessingLogs = m_isProcessingLogs;
            }

            //check to see if more events were added while the logger was processing the last batch
            lock (m_loggerQueueSync)
            {
                queueCount = m_loggerQueue.Count;
            }                

            if (queueCount == 0 && !isProcessingLogs)
                break;

            //since something is in the queue, reset the signal so we will not keep looping

            Thread.Sleep(400);
        }
    }
  • 1
    Welcome to Stack Overflow! We don't work like a traditional forum here. If you have an update, you can (and should) edit your original post with the new information/corrections instead of adding a new answer. – Pops Nov 26 '11 at 01:41
1

Just an update:

Using enteprise library 5.0 with .NET 4.0 it can easily be done by:

static public void LogMessageAsync(LogEntry logEntry)
{
    Task.Factory.StartNew(() => LogMessage(logEntry)); 
}

See: http://randypaulo.wordpress.com/2011/07/28/c-enterprise-library-asynchronous-logging/

Leo
  • 37,640
  • 8
  • 75
  • 100
John
  • 35
  • 1
  • 3
    This adds a thread per log message which *can be* very costly and counter productive. As @spoon16 mentioned in the question, queuing writes in a bulk manner. – M.Babcock Sep 17 '14 at 01:21
  • @M.Babcock when you are saying this add a thread per log message, you mean new thread? – Mukund Nov 20 '14 at 12:17
  • @Mukund - Yes, If logging is found to take more than 200-300 cpu cycles, it is most certainly executed in a new thread. – Lalman Jun 10 '15 at 10:38
  • I upvoted this, but to tell you, ideally, potentially expensive IO operations shouldn't use `Task.Run` like this. It is essentially turning a synchronous call to a fake asynchronous method. One should rely on a true asynchronous method if the library supports it and call it using async/await keywords (post C# 5.0). See http://blog.stephencleary.com/2013/11/taskrun-etiquette-examples-using.html and http://blogs.msdn.com/b/pfxteam/archive/2012/03/24/10287244.aspx. Just saying this is OK, does it's job, but not ideal. – nawfal Mar 24 '16 at 06:55
  • 3
    This could be very stressful to thread pool. IMHO not a good idea. – Michal Dobrodenka May 02 '16 at 07:58
  • @M.Babcock,@Mukund - it is incorrect to say "a thread per log message". Rather it is a Task per log message. Orchestration of Tasks in TPL is controlled by the ThreadPool which might or might not create a Thread if necessary. As Michal points out, this implementation is will cause great stress on the ThreadPool. A preferable solution is to use a single Task (for all messages). – Dave Black Sep 25 '17 at 21:26
0

An extra level of indirection may help here.

Your first async method call can put messages onto a synchonized Queue and set an event -- so the locks are happening in the thread-pool, not on your worker threads -- and then have yet another thread pulling messages off the queue when the event is raised.

Steve Gilham
  • 11,237
  • 3
  • 31
  • 37
0

If you log something on a separate thread, the message may not be written if the application crashes, which makes it rather useless.

The reason goes why you should always flush after every written entry.

leppie
  • 115,091
  • 17
  • 196
  • 297
  • That's true, I don't use this method for exception logging or other critical logging. I use it only for logging informational status messages describing successfully executed operations. These messages tend to occur much more often and I need to minimize their cost. – Eric Schoonover Jul 25 '09 at 08:26
0

If what you have in mind is a SHARED queue, then I think you are going to have to synchronize the writes to it, the pushes and the pops.

But, I still think it's worth aiming at the shared queue design. In comparison to the IO of logging and probably in comparison to the other work your app is doing, the brief amount of blocking for the pushes and the pops will probably not be significant.

Corey Trager
  • 22,649
  • 18
  • 83
  • 121