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.

For the m_listeners BlockingCollection<>, I know it's a bit hacky right now, but I'm not sure what to use instead (List<>, Dictionary<>, something else?). I want to be able to add/remove listeners at the same time as I may be broadcasting to those listeners without throwing null reference exceptions. For example add a listener for a TextBox when I open a sub-form and remove the listener when I close the sub-form without throwing an error because the TextBox doesn't exist or the delegate has become null, etc.

WindowsFormsApplication1 containing:

  • Form1, register OnLoad event
  • button1, register OnClick event
  • button2, register OnClick event
  • textbox1, enable multiline property and expand to show several lines at a time
using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Text;
using System.Windows.Forms;
using System.Threading;
using System.Threading.Tasks;
using System.Collections.Concurrent;
using System.IO;

namespace WindowsFormsApplication1
{
    public partial class Form1 : Form
    {
        //Fields
        private volatile bool cancel1 = false;
        private volatile bool cancel2 = false;
        private volatile bool cancel3 = false;
        private volatile bool cancel4 = false;

        public Form1()
        {
            InitializeComponent();
        }

        //Event Handlers
        private void button1_Click(object sender, EventArgs e)
        {
            //Automated Producers
            if (!cancel1)
            {
                Task.Factory.StartNew(() =>
                {
                    int count = 0;
                    while (!cancel1)
                    {
                        Log.Append("File", "log to file " + count++);
                        Thread.Sleep(100);
                    }
                    cancel1 = false;
                });
            }

            if (!cancel2)
            {
                Task.Factory.StartNew(() =>
                {
                    int count = 0;
                    while (!cancel2)
                    {
                        Log.Append("GUI", "log to GUI " + count++);
                        Thread.Sleep(200);
                    }
                    cancel2 = false;
                });
            }

            if (!cancel3)
            {
                Task.Factory.StartNew(() =>
                {
                    int count = 0;
                    while (!cancel3)
                    {
                        Log.Append("Error", "log to Error " + count++);
                        Thread.Sleep(300);
                    }
                    cancel3 = false;
                });
            }

            if (!cancel4)
            {
                Task.Factory.StartNew(() =>
                {
                    int count = 0;
                    while (!cancel4)
                    {
                        Log.Append("", "log to console " + count++);
                        Thread.Sleep(400);
                    }
                    cancel4 = false;
                });
            }
        }

        private void button2_Click(object sender, EventArgs e)
        {
            //Cancel Producers
            cancel1 = true;
            cancel2 = true;
            cancel3 = true;
            cancel4 = true;
        }

        private void Form1_Load(object sender, EventArgs e)
        {
            //Delete Old Files
            string LogFile = Directory.GetCurrentDirectory() + "\\Log.txt";
            if (File.Exists(LogFile))
                File.Delete(LogFile);

            string VerboseLog = Directory.GetCurrentDirectory() + "\\VerboseLog.txt";
            if (File.Exists(VerboseLog))
                File.Delete(VerboseLog);

            //Add Consumer Callback methods to Logger class
            //Append to File
            Log.RegisterWriter(
                new Action<string, string>((tag, entry) =>
                {
                    if (tag == "File")
                    {
                        using (TextWriter Stream = new StreamWriter(LogFile, true))
                        {
                            Stream.WriteLine(entry);
                        }
                    }
                }));

            //Append to different file
            Log.RegisterWriter(
                new Action<string, string>((tag, entry) =>
                {
                    using (TextWriter Stream = new StreamWriter(VerboseLog, true))
                    {
                        Stream.WriteLine(DateTime.Now + ":\t" + entry);
                    }
                }));

            //Append to Console
            Log.RegisterWriter(
                new Action<string, string>((tag, entry) =>
                {
                    Console.WriteLine(entry);
                }));

            //Append to multiline textBox1
            Log.RegisterWriter(
                new Action<string, string>((tag, entry) =>
                {
                    if (tag == "GUI")
                    {
                        entry += "\r\n";
                        if (this.InvokeRequired)
                        {
                            this.BeginInvoke(new Action<string>(textBox1.AppendText), new object[] { entry });
                            return;
                        }
                        else
                        {
                            //Under the circumstances, this never occurs. An invoke is always required.
                            textBox1.AppendText(entry);
                            return;
                        }
                    }
                }));
        }
    }

    public static class Log
    {
        //Fields
        private static BlockingCollection<Tuple<string, string>> m_logItems;
        private static BlockingCollection<Action<string, string>> m_listeners;

        //Methods
        public static void Append(string p_tag, string p_text)
        {
            //Add Log Entry
            m_logItems.Add(new Tuple<string, string>(p_tag, p_text));
        }

        public static void RegisterWriter(Action<string, string> p_callback)
        {
            //Add callback method to list
            m_listeners.Add(p_callback);
        }

        //Constructor
        static Log()
        {
            //Init Blocking Lists
            m_listeners = new BlockingCollection<Action<string, string>>();
            m_logItems = new BlockingCollection<Tuple<string, string>>();

            //Begin Log Entry Consumer Task
            Task.Factory.StartNew(() =>
            {
                //Consume as Log Entries are added to the collection
                foreach (var logentry in m_logItems.GetConsumingEnumerable())
                {
                    //Broadcast to each listener
                    foreach (var callback in m_listeners)
                    {
                        callback(logentry.Item1, logentry.Item2);
                    }
                }
            });
        }
    }
}

I didn't want to use a third-party logging library (partially to understand these mechanisms better) and just wanted to create a very simple way to post items from pretty much anywhere in my code (any thread) to various outputs (files, textboxes, the console, etc.).

The frequency of logging in the real application is much much slower than demonstrated here (i.e. it should never produce more than can be consumed under real-world conditions).

share|improve this question
    
You don't ever need to unregister a callback? –  svick Feb 12 '13 at 0:45
    
Yes, I will want to be able to unregister a callback. It's just that BlockingCollection is a bad choice for the m_listeners as it has no remove method that lets me unregister a specific item. I don't know what can be used instead that is still thread-safe. –  Josh W Feb 12 '13 at 1:10
3  
@JoshW You don't need to necessarily use a thread-safe collection. You can simply introduce your own lock with a private locking object wrapping access to the underlying collection. –  Chris Sinclair Feb 12 '13 at 2:20
    
This whole setting makes little sense to me. Why don't expose a single event from Log, and just fire it whenever someone logs something? You don't need any 'thread safety' to do that - just invoke in the handler. –  avip Feb 12 '13 at 4:48
    
@avip I looked into your suggestion and tried to implement it, however I can't invoke because I am not necessarily using any Controls in the Log class. I've never implemented the ISynchronizeInvoke interface, so I'm not sure that's any easier at this point. –  Josh W Feb 12 '13 at 15:25

1 Answer 1

up vote 1 down vote accepted

Answering my own question here based off suggestions in the question comments.

This seems to work decently well. Good to have debugger Output Console open and see the behaviour of registering/unregistering logs to the GUI textbox (especially multiple clicks).

I changed from using a BlockingCollection for the m_listeners object to instead use a normal Dictionary<>. Locks were added around each section of code that could (or does) change the m_listeners dictionary.

As a side note I changed from using a few booleans to end the Producer tasks to instead use CancellationTokens as that's the built mechanism for that sort of thing.

Form requires 4 buttons and a textbox

  • Form1, register OnLoad
  • button1, register OnClick
  • button2, register OnClick
  • button3, register OnClick
  • button4, register OnClick
  • textbox1, enable multiline property and expand to show several lines at a time
using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Text;
using System.Windows.Forms;
using System.Threading;
using System.Threading.Tasks;
using System.Collections.Concurrent;
using System.IO;

namespace WindowsFormsApplication1
{
    public partial class Form1 : Form
    {
        //Fields
        CancellationTokenSource m_tokenSource = new CancellationTokenSource();
        List<int> m_logtoGUIHandle = new List<int>();

        //Methods
        public Form1()
        {
            InitializeComponent();
        }
        private void Form1_Load(object sender, EventArgs e)
        {
            //Button Descriptions
            button1.Text = "Generate";
            button2.Text = "Cancel";
            button3.Text = "Register";
            button4.Text = "Unregister";

            //Delete Old Files
            string LogFile = Directory.GetCurrentDirectory() + "\\Log.txt";
            if (File.Exists(LogFile))
                File.Delete(LogFile);

            string VerboseLog = Directory.GetCurrentDirectory() + "\\VerboseLog.txt";
            if (File.Exists(VerboseLog))
                File.Delete(VerboseLog);

            //Add Consumer Callback methods to Logger class
            //Append to File
            Log.RegisterWriter(new Action<string, string>((tag, entry) =>
            {
                if (tag == "File")
                {
                    using (TextWriter Stream = new StreamWriter(LogFile, true))
                    {
                        Stream.WriteLine(entry);
                    }
                }
            }));

            //Append to different file
            Log.RegisterWriter(new Action<string, string>((tag, entry) =>
            {
                using (TextWriter Stream = new StreamWriter(VerboseLog, true))
                {
                    Stream.WriteLine(DateTime.Now + ":\t" + entry);
                }
            }));

            //Append to Console
            Log.RegisterWriter(new Action<string, string>((tag, entry) =>
            {
                Console.WriteLine(entry);
            }));
        }

        //Event Handlers
        private void button1_Click(object sender, EventArgs e)
        {
            CancellationToken ct = m_tokenSource.Token;

            //Automated Producers
            Task.Factory.StartNew(() =>
            {
                int count = 0;
                while (!ct.IsCancellationRequested)
                {
                    //Log Event
                    Log.Append("File", "log to file " + count++);
                    Thread.Sleep(100);
                }
            }, ct);

            Task.Factory.StartNew(() =>
            {
                int count = 0;
                while (!ct.IsCancellationRequested)
                {
                    Log.Append("GUI", "log to GUI " + count++);
                    Thread.Sleep(200);
                }
            }, ct);

            Task.Factory.StartNew(() =>
            {
                int count = 0;
                while (!ct.IsCancellationRequested)
                {
                    Log.Append("Error", "log to Error " + count++);
                    Thread.Sleep(300);
                }
            }, ct);

            Task.Factory.StartNew(() =>
            {
                int count = 0;
                while (!ct.IsCancellationRequested)
                {
                    Log.Append("", "log to console " + count++);
                    Thread.Sleep(400);
                }
            }, ct);
        }
        private void button2_Click(object sender, EventArgs e)
        {
            //Cancel Producers
            m_tokenSource.Cancel();
            //Required or Producers will immediately exit in cancelled state
            m_tokenSource = new CancellationTokenSource();
        }
        private void button3_Click(object sender, EventArgs e)
        {
            //Append to multiline textBox1
            m_logtoGUIHandle.Add(Log.RegisterWriter(
                 new Action<string, string>((tag, entry) =>
                 {
                     if (tag == "GUI")
                     {
                         entry += "\r\n";
                         if (this.InvokeRequired)
                         {
                             this.BeginInvoke(new Action<string>(textBox1.AppendText), new object[] { entry });
                             return;
                         }
                         else
                         {
                             //Under the circumstances, this never occurs. An invoke is always required.
                             textBox1.AppendText(entry);
                             return;
                         }
                     }
                 })));
        }
        private void button4_Click(object sender, EventArgs e)
        {
            foreach (var h in m_logtoGUIHandle)
            {
                Log.UnregisterWriter(h);
            }
        }
    }

    public static class Log
    {
        //Fields
        private static Dictionary<int, Action<string, string>> m_listeners;
        private static BlockingCollection<Tuple<string, string>> m_logItems;
        private static int handle = 0;
        private static object syncObject = new object();

        //Methods
        public static void Append(string p_tag, string p_text)
        {
            //Add Log Entry
            m_logItems.Add(new Tuple<string, string>(p_tag, p_text));
        }
        public static int RegisterWriter(Action<string, string> p_callback)
        {
            //Add callback method to list
            lock (syncObject)
            {
                handle++;
                m_listeners[handle] = p_callback;
                return handle;
            }
        }
        public static void UnregisterWriter(int p_handle)
        {
            //Remove callback method from list
            lock (syncObject)
            {
                if (m_listeners.ContainsKey(p_handle))
                {
                    m_listeners.Remove(p_handle);
                }
            }
        }

        //Constructor
        static Log()
        {
            //Init
            m_listeners = new Dictionary<int, Action<string, string>>();
            m_logItems = new BlockingCollection<Tuple<string, string>>();

            //Begin Log Entry Consumer Task
            Task.Factory.StartNew(() =>
            {
                //Continuously consumes Log Entries added to the collection, blocking-wait if empty
                foreach (var logentry in m_logItems.GetConsumingEnumerable())
                {
                    //Lock to prevent enumerator changes to callback dictionary
                    lock (syncObject)
                    {
                        //Broadcast to each listener
                        foreach (var callback in m_listeners)
                        {
                            callback.Value(logentry.Item1, logentry.Item2);
                        }
                    }
                }
            });
        }
    }
}
share|improve this answer
1  
Welcome, Josh! Could you add a paragraph explaining what you changed (the lock) so that this makes sense out of context? –  codesparkle Feb 12 '13 at 19:12
    
@codesparkle Thanks, I added some detail about the changes I made to get this working nicely. –  Josh W Feb 12 '13 at 19:47

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.