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.

Because I needed to execute some actions one by one in seperate thread (not to block GUI) and I couldn't use Task.ContinueWith from .NET 4.0 I decided to write it by myself.

Here is how it evolved thanks to your suggestions.

public class ActionQueue
{
    private Thread _thread;
    private bool _isProcessed = false;
    private object _queueSync = new object();
    private readonly Queue<Action> _actions = new Queue<Action>();
    private SynchronizationContext _context;

    /// <summary>
    /// Occurs when one of executed action throws unhandled exception.
    /// </summary>
    public event CrossThreadExceptionEventHandler ExceptionOccured;

    /// <summary>
    /// Occurs when all actions in queue are finished.
    /// </summary>
    public event EventHandler ProcessingFinished;

    /// <summary>
    /// Gets enqueued actions.
    /// </summary>
    public IEnumerable<Action> Actions
    {
        get
        {
            lock (_queueSync)
            {
                return new ReadOnlyCollection<Action>(_actions.ToList());
            }
        }
    }

    protected virtual void Execute()
    {
        _isProcessed = true;

        try
        {
            while (true)
            {
                Action action = null;

                lock (_queueSync)
                {
                    if (_actions.Count == 0)
                        break;
                    else
                        action = _actions.Dequeue();
                }

                action.Invoke();
            }

            if (ProcessingFinished != null)
            {
                _context.Send(s => ProcessingFinished(this, EventArgs.Empty), null);
            }
        }
        catch (ThreadAbortException)
        {
            // Execution aborted
        }
        catch (Exception ex)
        {
            if (ExceptionOccured != null)
            {
                _context.Send(s => ExceptionOccured(this, new CrossThreadExceptionEventArgs(ex)), null);
            }
        }
        finally
        {
            _isProcessed = false;
        }
    }

    /// <summary>
    /// Starts processing current queue.
    /// </summary>
    /// <returns>Returns true if execution was started.</returns>
    public virtual bool Process()
    {
        if (!_isProcessed)
        {
            _context = SynchronizationContext.Current;

            _thread = new Thread(Execute);
            _thread.Start();

            return true;
        }
        else
        {
            return false;
        }
    }

    /// <summary>
    /// Enqueues action to process.
    /// </summary>
    /// <param name="action">Action to enqueue.</param>
    public void Enqueue(Action action)
    {
        lock (_queueSync)
        {
            _actions.Enqueue(action);
        }
    }

    /// <summary>
    /// Clears queue.
    /// </summary>
    public void Clear()
    {
        lock (_queueSync)
        {
            _actions.Clear();
        }
    }

    /// <summary>
    /// Aborts execution of current queue.
    /// </summary>
    /// <returns>Returns true if execution was aborted.</returns>
    public bool Abort()
    {
        if (_isProcessed)
        {
            _thread.Abort();
            return true;
        }
        else
        {
            return false;
        }
    }
}

public delegate void CrossThreadExceptionEventHandler(object sender, CrossThreadExceptionEventArgs e);

public class CrossThreadExceptionEventArgs : EventArgs
{
    public CrossThreadExceptionEventArgs(Exception exception)
    {
        this.Exception = exception;
    }

    public Exception Exception
    {
        get;
        set;
    }
}

Here is some usage example:

ActionQueue queue = new ActionQueue();

queue.Enqueue(SomeMethod);
queue.Process();

foreach(var item in collection)
{
    var itemToProcess = item;
    queue.Enqueue(() => SomeMethod(itemToProcess));
}

queue.Process();


Edits:

  • Implemented observable pattern
  • Synchronization context added
  • IsProcessed property removed
share|improve this question
add comment

4 Answers

Since the class itself does very little (runs a bunch of methods on a separate thread), there is little justification in using it, IMHO.

This statement would basically do the same thing (on a ThreadPool, that is):

ThreadPool.QueueUserWorkItem(s =>
{
    SomeMethod();
    DoSomething();
    AndSomeMore();
});

If it included stuff like progress updating, exception handling and finalization events, then it could make more sense to use it:

interface IActionQueue
{
    void Start();

    // if you know the list count, you can provide some progress info
    event Action<Double> ProgressChanged;

    // there is no way to catch an exception on a separate thread, so 
    // this would be neat
    event Action<Exception> ExceptionHappened;

    // this can be useful also
    event Action Finished;
}
share|improve this answer
    
I think that justification in using it is the possibility to reuse it and expand in the future. You think that isn't enough? :) –  rotman Dec 14 '11 at 13:26
    
And I thought that ThreadPool is used rather to do some work in parallel. Am I wrong? Does it execute actions one by one? –  rotman Dec 14 '11 at 13:30
    
Oh, sorry - now I see that you put it in a single lambda :) You are right then. Thank you. –  rotman Dec 14 '11 at 13:39
    
But wait... I need to enqueue some actions from different places and in different moments and then start them from another so the code you provided is not enough. Of course, I can build a queue and process it using ThreadPool but don't you think that using ActionQueue is better idea? –  rotman Dec 14 '11 at 13:46
    
@rotman: ok, my answer was based on your first example. If you do need to create this list from several places, using a separate class might make it more explicit. But you should then consider suggestions made by Slade and Jesse: I would not expose the mutable list to the outside world, if there is a chance that it may be iterated on a separate thread. The simplest way would be to change the list into a Queue, make it private, and expose a public Add method which should lock while enqueuing. –  Groo Dec 15 '11 at 13:04
show 1 more comment

Your approach is not ThreadSafe, replace the Queue with ConcurrentQueue or add locking around all the Enqueue, Dequeue, and Count members. Also, I would avoid throwing an exception if the Queue is still processing. This condition is something that is largely a private concern of your class.

share|improve this answer
    
I can't use ConcurrentQueue because it's .NET 3.5 so I introduced locks. Thanks for that. Maybe throwing exception it's the best solution (do you have any suggestions?) but I rather disagree that state of being processed is a private concern of the class. What if some other object wants to check if queue has ended the work? I can imagine such situation. –  rotman Dec 16 '11 at 13:45
1  
In both cases where your throwing an exception I would just exit silently. Although I don't have the full context, it just seems like the caller shouldn't need to the know about the inner workings of this class; at least not at that level. Having said that, if the caller needs to be aware of whether or not their invokation led to the initiation of any real action, you could simply return with a bool or enum indicating what they triggered; if anything. –  JoeGeeky Dec 16 '11 at 19:59
1  
Sorry, I forgot to answer one of your questions. If callers need to be notified when a queued item is processed, there are more efficient ways of achieving this. For example, you could use an Observer Pattern and let callers subscribe to the working state of the Queue (Ex. OnProcessingQueuedItems, OnQueuedItemProcessed, OnProcessingQueueCompleted, etc...). Alternatively, each caller could register a callback with the request and be notified when their instance is processed. In any case, using exceptions such as I saw above just isn't a great way to communicate what you've indicated –  JoeGeeky Dec 16 '11 at 20:09
    
I totally agreed with your suggestions. I can find updated version above. Thanks. –  rotman Dec 19 '11 at 13:14
add comment

If you are using the ActionQueue simply for invoking a collection of actions one-by-one one another thread, then there isn't really much you can say over the implementation (other than the point already made by Groo).

However, if you wish to have this ActionQueue running on another thread, while accepting requests to queue another action to execute (so you can execute actions asynchronously), then you may wish to look into using a combination of Queue<Action>, a background worker thread and AutoResetEvent. I won't write the full implementation of that here, as I don't know if that is what you want, but if you do then I would be happy to edit this answer.

share|improve this answer
add comment

Don't expose your concrete lists:

public class ActionQueue
{
    private readonly IList<Action> actions = new List<Action>();

    public IEnumerable<Action> Actions
    {
        get
        {
            return new ReadOnlyCollection<Action>(actions);
        }
    }

    public void Add(Action action)
    {
        this.actions.Add(action);
    }

    protected virtual void Execute()
    {
        foreach (var action in this.actions)
        {
            action.Invoke();
        }
    }

    public virtual void Start()
    {
        var thread = new Thread(Execute);

        thread.Start();
    }
}
share|improve this answer
2  
I wouldn't use the IList interface in this case if you are wanting to restrict access to the Actions property. Instead, I'd maintain it as List inside of the class and change the return type of the Actions property to IEnumerable<Action>. Then still use the ReadOnlyCollection<Action>. –  Samuel Slade Dec 14 '11 at 15:08
    
I'll buy that.. –  Jesse C. Slicer Dec 14 '11 at 15:29
    
I took into account your suggestions. New version above. –  rotman Dec 15 '11 at 15:22
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.