Is this Agent/Actor implementation issue free?

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 : IDisposable
{
private Thread thread;
private Queue messageQueue;
private bool quit;

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

this.thread.Start();
}

///
/// 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).
///
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,

Few minor points:

• Generic type parameter should be called ` 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.

StackExchange Code Review Q#21337, answer score: 4