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.

I implemented this Agent class for a project recently and was wondering if I could get some other eyes to look at it -- I'm currently the only developer where I work so I can't exactly ask someone here to do it.

I'm pretty sure it's correct, but then I'm too close to it.

public abstract class Agent<M> : IDisposable
{
    private Thread thread;
    private Queue<M> messageQueue;
    private bool quit;

    public Agent()
    {
        this.thread = new Thread(new ThreadStart(AgentThreadBody));
        this.messageQueue = new Queue<M>();
        this.quit = false;

        this.thread.Start();
    }

    /// <summary>
    /// Stops the message-handling thread.
    /// 
    /// Do not call this method from within the message-
    /// handling method, or it will result in a deadlock
    /// (because this method waits for the message-handling
    /// thread to stop).
    /// </summary>
    public virtual void Dispose()
    {
        this.quit = true;
        this.thread.Interrupt();
        this.thread.Join();
        this.thread = null;
        // clear messageQueue before nulling?
        // (would do this to dispose queued items)
        this.messageQueue = null;
    }

    public void QueueMessage(M message)
    {
        lock (this.messageQueue)
        {
            this.messageQueue.Enqueue(message);
            this.thread.Interrupt();
        }
    }

    private void AgentThreadBody()
    {
        while (!this.quit)
        {
            M message = default(M);
            bool messageAvailable = false;

            lock (messageQueue)
            {
                if (messageQueue.Count > 0)
                {
                    message = messageQueue.Dequeue();
                    messageAvailable = true;
                }
            }

            try
            {
                if (!messageAvailable)
                {
                    // if the Interrupt() method was
                    // called before we sleep or is
                    // called while we're sleeping,
                    // this will throw:
                    // ThreadInterruptedException
                    Thread.Sleep(Timeout.Infinite);
                }
            }
            catch (ThreadInterruptedException)
            {
                // we have a new message to handle,
                // so get it, or we've been told
                // to quit.
                continue;
            }

            ProcessMessage(message);
        }
    }

    protected abstract void ProcessMessage(M message);
}

Also, are there any special considerations you can think of that should be made by a class inheriting from this base class? (I can't think of any.)

I could add start/stop methods, but at the moment they're not needed.

Btw, I have to use .NET 3.5.

share|improve this question
add comment

3 Answers 3

up vote 2 down vote accepted

Instead of sleeping for short periods or using Thread.Interrupt() it's better to use waithandles, ManualResetEvent in your case. I don't have Visual Studio at hand, but the following example should give you the basic idea (most of the complexity will be gone if you start using .NET 4 or 4.5, in particular BlockingCollection<T>):

public abstract class Agent<TMessage> : IDisposable
{
    private readonly Thread _thread = new Thread(AgentThreadBody);
    private readonly Queue<TMessage> _messageQueue = new Queue<TMessage>();
    private readonly ManualResetEvent _waitHandle = new ManualResetEvent(false);
    private volatile bool _quit;
    private bool _disposed;

    protected Agent()
    {
        _thread.Start();
    }

    public void QueueMessage(M message)
    {
        lock (_messageQueue)
        {
            _messageQueue.Enqueue(message);
            _waitHandle.Set();
        }
    }

    private void AgentThreadBody()
    {
        while (!_quit)
        {
            _waitHandle.WaitOne();
            TMessage message;

            lock (messageQueue)
            {
                if (messageQueue.Count == 0)
                {
                    _waitHandle.Reset();
                    continue;
                }

                message = messageQueue.Dequeue();
            }

            ProcessMessage(message);
        }
    }

    protected abstract void ProcessMessage(M message);

    /// <summary>
    /// Stops the message-handling thread.
    /// 
    /// Do not call this method from within the message-
    /// handling method, or it will result in a deadlock
    /// (because this method waits for the message-handling
    /// thread to stop).
    /// </summary>
    public void Dispose()
    {
        Dispose(true);
        GC.SupressFinalize(this);
    }

    protected virtual void Dispose(bool disposing)
    {
        if (_disposed || !disposing)
            return;

        _quit = true;
        _waitHandle.Set();
        _thread.Join();
        _waitHandle.Dispose();
        _disposed = true;
    }
}
share|improve this answer
    
Thanks, this method is working well! I'm not that familiar with all of .NET's thread synchronization methods, and I was in a bit of a rush, so I probably wouldn't have come up with using this any time soon. If I were using .NET 4, I would have been doing this in F# and used a MailboxProcessor (or Agent from the FSharpx library) instead of rolling my own. (I think I read somewhere that F# has been backported to .NET 2 but I haven't had time to investigate using it.) Thanks again! –  paul Mar 1 '13 at 21:25
add comment

Few minor points:

  • Generic type parameter should be called <T> as per convention.
  • public constructors in abstract classes make no sense - make it protected.
  • Make thread and messageQueue readonly and don't set them to null in the Dispose() method.
  • Better yet, implement the Disposable Pattern correctly. Though note, I do disagree with their setting of the IDisposable members to null. It's really not necessary at all; but calling Dispose() is. Their _disposed class member conveys enough information necessary.
  • You should also lock on a dedicated locking class member when accessing your quit class member since it is accessed via multiple threads.
  • Possibly wrap your call to ProcessMessage(message) in a try..catch block with appropriate error handling (or not, as the case could be). Unless, of course, you trust any subclasses from doing nasty things in their override of it.

Just my initial thoughts. Hope they help.

share|improve this answer
1  
Thanks for the tips! In response: (1) No reason not to go w/ convention on this one. (2) Good call on protected! (3,4) I'll have to look into proper handling of this further -- thanks for the link. (5) No need to lock quit since it's a bool and access is atomic. I did add volatile though. (Related.) (6) I had considered that; I'm not sure if I want to swallow exceptions there or just let them fall through and crash. (Logging them would be difficult in this application.) Thanks again! –  paul Feb 5 '13 at 16:03
    
I found this article on properly implementing the dispose pattern a bit clearer than the article to which you linked, in case you need a good link for someone in the future. –  paul Feb 19 '13 at 21:13
    
@Jesse, concerning "don't set fields to null". Obviously null'ing fields will not free the memory immediately, and it will not help GC in case when the code is written correctly. But it will help in reducing memory leaks if some references to main (owner) object will remain after disposition thus preventing it from being garbage-collected. In this case null'ing the references allows nested objects to be garbage-collected even though parent object will stay in memory. –  almaz Feb 27 '13 at 22:28
    
@almaz I disagree completely. Your object which is being Dispose'd, as the owner of those fields, will be unused by your program and eligible for garbage collection directly post-Dispose(). Those fields will then also be GC'd if they have no other references (they shouldn't). –  Jesse C. Slicer Feb 27 '13 at 22:35
    
I'd rather use TMessage than the meaningless T. Just T is mostly applicable in situations where the parameter has no real meaning, such as collections. –  CodesInChaos Feb 27 '13 at 22:54
show 1 more comment

No, this implementation is not issue free!

The Thread.Interrupt() method apparently causes a ThreadInterruptedException to be thrown whenever the thread is blocked. The documentation led me to believe that it was only thrown under certain circumstances:

If this thread is not currently blocked in a wait, sleep, or join state, [emph mine] it will be interrupted when it next begins to block.

However, a lot more can block a thread than you'd think! I first started seeing what I thought were rogue instances of this exception in a derived agent instance when accessing DateTime.Now! (This is apparently due to the Now property requiring blocking IO to obtain a value.)

Locking can also cause a thread to block, as lock gets translated into Monitors (which block).

Looks like I'll have to go with a solution that just Sleep()s for short periods of time instead of waiting indefinitely until new input became available :-/

share|improve this answer
add comment

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.