6
\$\begingroup\$

Recently, I needed a class that could execute code in a FIFO fashion in order to update parts of a WinForms or WPF UI which did not block the UI Thread and left the UI responsive to any interaction the user did on it.

To achieve that objective, I developed the following code, which meets its requirements and performs decently enough to not be a burden.

using System;
using System.Collections.Concurrent;
using System.ComponentModel;
using System.Threading;

namespace Rayffer.PersonalPortfolio.QueueManagers
{
    public class BackgroundWorkerActionQueueManager : IDisposable
    {
        public bool IsBusy { get; private set; }

        private BlockingCollection<Action> actionQueue;
        private readonly BackgroundWorker backgroundWorker;
        private CancellationTokenSource cancellationTokenSource;

        public BackgroundWorkerActionQueueManager()
        {
            backgroundWorker = new BackgroundWorker();
            actionQueue = new BlockingCollection<Action>();
            cancellationTokenSource = new CancellationTokenSource();
            backgroundWorker.WorkerSupportsCancellation = true;
            backgroundWorker.DoWork += BackgroundWorker_ManageQueue;
            backgroundWorker.RunWorkerAsync();
        }

        public void EnqueueAction(Action actionToEnqueue)
        {
            actionQueue.Add(actionToEnqueue);
        }

        private void BackgroundWorker_ManageQueue(object sender, DoWorkEventArgs e)
        {
            while (!backgroundWorker.CancellationPending)
            {
                Action actionToPerform = null;
                try
                {
                    IsBusy = false;
                    actionToPerform = actionQueue.Take(cancellationTokenSource.Token);
                    IsBusy = true;
                }
                catch (OperationCanceledException ex)
                {
                    // This exception control intends to capture only when the cancellationToken has been requested to cancel
                    break;
                }
                if (actionToPerform != null)
                {
                    actionToPerform.Invoke();
                    actionToPerform = null;
                }
            }
        }

        private bool disposedValue = false; // To detect redundant calls

        protected virtual void Dispose(bool disposing)
        {
            if (!disposedValue)
            {
                if (disposing)
                {
                    backgroundWorker.CancelAsync();
                    cancellationTokenSource.Cancel();
                    backgroundWorker.DoWork -= BackgroundWorker_ManageQueue;
                    cancellationTokenSource.Dispose();
                    backgroundWorker.Dispose();
                }

                disposedValue = true;
            }
        }

        public void Dispose()
        {
            Dispose(true);
        }
    }
}

This code can be used to perform any action enqueued, from doing a simple sum, to manipulating members of a class ensuring that the operations are carried out in the order they are enqueued.

I was wondering which optimisations I can apply to this code or if there are any bad practices or missusages of the classes.

An example of use would be the following

public class BackGroundWorkerActionQueueManagerTest
{
    private BackgroundWorkerActionQueueManager sorterActionQueueManager = new BackgroundWorkerActionQueueManager ();
    private BackgroundWorkerActionQueueManager sorterVisualizerActionQueueManager = new BackgroundWorkerActionQueueManager ();
    private bool sorterHasEnded = false;
    public Sorter sorter { get; }


    public void StartSorterQueueTest(int[] arrayToSort)
    {
        sorterActionQueueManager.EnqueueAction(() =>
            {
                sorter.SortAscending(arrayToSort);
                sorterHasEnded = true;
            });
    }

    public void StartSorterQueueVisualisation()
    {
        sorterVisualizerActionQueueManager.EnqueueAction(() =>
            {
                while(!sorterHasEnded)
                {
                    Thread.Sleep(100);
                }
                // Perform operations to visualise the sorting here
            });
    }
}
```
\$\endgroup\$
  • 1
    \$\begingroup\$ Could you provide an example or explanation of how you would expect to use this? In particular, I'm clear why you have IsBusy. \$\endgroup\$ – VisualMelon Sep 10 at 13:26
  • 2
    \$\begingroup\$ I added an example of how I expect the class to be used, hope it is enough as it is just a proof of concept. \$\endgroup\$ – Oscar Guillamon Sep 10 at 13:35
  • 1
    \$\begingroup\$ I have rolled back your last edit because it's not allowed to change the code when answers have been posted. This could invalide them. \$\endgroup\$ – t3chb0t Sep 12 at 9:11
  • \$\begingroup\$ @t3chb0t how can I edit correctly the question to reflect the answers in order to provide an updated code, should I answer my own question? \$\endgroup\$ – Oscar Guillamon Sep 12 at 9:13
  • \$\begingroup\$ Yes, a self-answer would be the right way to post the updated code. Keep in mind, however, that it also has to be like a review this means you should not just post the new code but also summarize the changes you've made. \$\endgroup\$ – t3chb0t Sep 12 at 9:15
5
\$\begingroup\$

I think, I would provide the CancellationToken to each action, in order to let them respond to a possible cancellation request. Especially if the action is long running it would be a good idea.

BlockingCollection<Action<CancellationToken>> actionQueue

            while(!sorterHasEnded)
            {
                Thread.Sleep(100);
            }

This tells me, that you are in the need for a mechanism that tells you when each queued action has finished. May be an event. You could then bind each action to a user defined Id of some kind and signal that, when the action is completed:

private BlockingCollection<ActionItem> actionQueue;


public void EnqueueAction(string id, Action<CancellationToken> actionToEnqueue)
{
  if (string.IsNullOrWhiteSpace(id)) new ArgumentException("Can not be empty or null", nameof(id));
  if (actionToEnqueue == null) new ArgumentNullException(nameof(actionToEnqueue));

  actionQueue.Add(new ActionItem(id, actionToEnqueue));
}

And the background worker could be as something like:

private void BackgroundWorker_ManageQueue(object sender, DoWorkEventArgs e)
{
  while (!backgroundWorker.CancellationPending)
  {
    ActionItem actionItem;
    try
    {
      IsBusy = false;
      actionItem = actionQueue.Take(cancellationTokenSource.Token);
      IsBusy = true;
    }
    catch (OperationCanceledException)
    {
      // This exception control intends to capture only when the cancellationToken has been requested to cancel
      break;
    }

    actionItem.Action(cancellationTokenSource.Token);
    OnActionEnded(actionItem.Id);
  }
}

public event EventHandler<ActionEndedEventArgs> ActionEnded;

private void OnActionEnded(string id)
{
  ActionEnded?.Invoke(this, new ActionEndedEventArgs(id));
}

Where ActionItem is defined as:

class ActionItem
{
  public ActionItem(string id, Action<CancellationToken> action)
  {
    Id = id;
    Action = action;
  }

  public string Id { get; }
  public Action<CancellationToken> Action { get; }
}

Maybe the ActionEnded event should be fired in a separate thread in order to not block the execution queue while a client responds to it?

\$\endgroup\$
  • \$\begingroup\$ How would you go about firing 'actionended' on a separate thread \$\endgroup\$ – Oscar Guillamon Sep 10 at 16:36
  • \$\begingroup\$ @OscarGuillamon: Wouldn't it be possible in a ThreadPool thread? \$\endgroup\$ – Henrik Hansen Sep 10 at 16:38
  • 1
    \$\begingroup\$ Yeah, that's what I was thinking about, Ive not worked a lot with threadpool threads, so I'll give it a shot and read about it \$\endgroup\$ – Oscar Guillamon Sep 10 at 16:39
7
\$\begingroup\$

EnqueueAction may want to throw an ObjectDisposedException if the queue is disposed, depending on the precise API you want.


I don't see the value of IsBusy, and it won't be cleared if the task is cancelled.


I little padding inside the while loop would make that code much easier to understand. I'd be temped to pull the logic for retrieving the next action out into its own method so that it can read more clearly:

if (TryGetNextAction(out Action nextAction))
{
    // invoke etc.
}
else
{
    break;
}

This will make it easier to extend and modify the conditions under which the worker should terminate. You could make it a local function if you want to prevent misuse.

For example, cancellationTokenSource.Token could throw an ObjectDisposedException, which should probably be a case where you fail cleanly, but handling this logic would clutter the existing method.


You never change any of your private members, so you might consider making them readonly. Alternatively you might consider setting them to null upon disposal. Implementation of IDispose looks standard, and I think disposing the BackgroundWorker there is OK, but I'm less than certain.


Some inline documentation would help to confirm the precise API this class is providing, and should clarify things like "if the object is disposed, then pending tasks are dropped", and "order of operations is FIFO as observed from a single thread", etc. BackGroundWorkerActionQueueManagerTest doesn't really elucidate its behaviour.

\$\endgroup\$
  • \$\begingroup\$ In which conditions the token can throw an object disposed exception? I hold a reference to it throughout the whole lifetime of the class, as it is intended only to cancel the .Take() \$\endgroup\$ – Oscar Guillamon Sep 10 at 16:39
  • \$\begingroup\$ It's a public class, other consumers could use it in a way the lifetime is handled differently than you intended, so you should guard against it. \$\endgroup\$ – dfhwze Sep 10 at 16:41
  • \$\begingroup\$ @OscarGuillamon disposing the CancellationTokenSource will invalided it, and the call to Token may duly fail. (i.e. var cts = new CancellationTokenSource(); cts.Dispose(); var t = cts.Token; will throw) \$\endgroup\$ – VisualMelon Sep 10 at 16:44
  • \$\begingroup\$ @dfhwze but the token is private, which is only disposed at the object's disposal itself, I fail to see what's wrong, can you further expand please? \$\endgroup\$ – Oscar Guillamon Sep 10 at 17:00
  • \$\begingroup\$ there can be a race condition in which Dispose already disposed the token while BackgroundWorker_ManageQueue still uses it one last time. \$\endgroup\$ – dfhwze Sep 10 at 17:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.