8
\$\begingroup\$

I have a Queue that needs to try to process when anything updates in it (add, remove or update).

However, I don't want any of the callers to wait while the processing is happening (either for the processing to happen or while the processing is happening).

This is what I came up with:

private static readonly SemaphoreSlim asyncLock = new SemaphoreSlim(1);
private async void ProcessQueue()
{
    // Lock this up so that one thread at a time can get through here.  
    // Others will do an async await until it is their turn.
    await asyncLock.WaitAsync();
    try
    {
        // Offload this to a background thread (so that the UI is not affected)
        var queueProcessingTask = Task.Run( () =>
        {
            var processingStuck = false;
            while (myQueue.Count >= 1 && !processingStuck)
            {
                // Get the next item
                var queueItem = myQueue.Peek();
                // Try to process this one. (ie DoStuff)
                processingStuck = ProcessQueueItem(queueItem);
                // If we processed successfully, then we can dequeue the item
                if (!processingStuck)
                    myQueue.Dequeue();
            }                
        });
        Task.WaitAll(queueProcessingTask);
    }
    finally
    {
        asyncLock.Release();
    }    
}

Is my Thread and async handling going to ensure that only 1 queueItem at a time can ever be "in the works"?

And will this avoid using resources from the UI thread?

\$\endgroup\$
  • 2
    \$\begingroup\$ Welcome to CR! Quick question: to the best of your knowledge, does the code work as expected? (just to make sure) \$\endgroup\$ – Mathieu Guindon Feb 26 '14 at 20:58
  • \$\begingroup\$ @Mat'sMug - I have not put it through all the paces. Should I delete it an re-post it when I am more confident of how well it works? \$\endgroup\$ – Vaccano Feb 26 '14 at 21:02
  • 2
    \$\begingroup\$ Peer reviews can find hidden bugs, that's not a problem. However if there's a known problem with your code, CR isn't the right place to get it fixed (that's SO's grounds ;) - if the code doesn't blow up and works as you'd expect it to work (to the best of your knowledge), then I wouldn't delete it. I've favorited this post, will come back to it later for sure ;) \$\endgroup\$ – Mathieu Guindon Feb 26 '14 at 21:07
  • 1
    \$\begingroup\$ What is the type/class of your myQueue item? Which thread (is it the UI thread) that's calling your ProcessQueue method? When behaviour do you want if ProcessQueueItem returns true? \$\endgroup\$ – ChrisW Feb 26 '14 at 22:11
  • \$\begingroup\$ @ChrisW - It is a custom type of mine. Has a few native types and a dictionary. The thread that calls it will likely be the UI Thread. (Though that is not guaranteed.) If ProcessQueueItems returns true then everything should stop. (The idea is that we would get user interaction that would update the item and cause another call of this method (that would succeed)). The problem is that there are a lot of threads going on, and so this method could be called many times and I need to make sure that only one item is being worked on at once. \$\endgroup\$ – Vaccano Feb 26 '14 at 23:31
3
\$\begingroup\$

The thread that calls it will likely be the UI Thread.

You're invoking Task.WaitAll which waits for the processing to finish.

However you said, "However, I don't want any of the callers to wait while the processing is happening (either for the processing to happen or while the processing is happening)."

Is this a contradiction?

Instead of creating short-lived tasks as a local variable inside a semaphore, perhaps it would be better to:

  • Create a single long-lived task (e.g. as a data member of your class)
  • Wake up the task when you enqueue something or fix a ProcessQueueItems problem (perhaps by setting a event which the task is waiting on)
  • Don't invoke Task.WaitAll until your program is shutting down
  • Put an exception handler inside the task so that the (single, supposedly long-lived) task can't permanently fail if one ProcessQueueItem call throws an exception

It is a custom type of mine. Has a few native types and a dictionary.

It needs to be a type like ConcurrentQueue<T> because you have one thread (the task) dequeueing from it while (presumably) other threads are enqueueing on it.

\$\endgroup\$
  • \$\begingroup\$ I am doing the WaitAll because I need my call to finish before I allow another caller to run the method. But, I see your point, I should be waiting async. That way the task can wait to release the asyncLock until the task finishes, but not block. About the myQueue, it is of Type Queue. I mistakenly thought you were asking about queueItem. I am not using ConcurrentQueue because the goal of my code is to ensure that more than one thread does not ever access the queue at once. Not for the safety of the queue, but because I end up printing things from the queue that MUST be in order. \$\endgroup\$ – Vaccano Feb 26 '14 at 23:56
  • \$\begingroup\$ If you want to use Queue instead of ConcurrentQueue then you need to use e.g. lock to make it thread-safe: acquire a lock before you dequeue from it, and acquire the same lock before you enqueue on it (to ensure that two different threads don't simultaneously enqueue and dequeue). \$\endgroup\$ – ChrisW Feb 26 '14 at 23:59
  • \$\begingroup\$ doesn't my SemaphoreSlim asyncLock do that? I put that in so that I could have a non-blocking lock. \$\endgroup\$ – Vaccano Feb 27 '14 at 0:02
  • \$\begingroup\$ What is the problem with two different threads doing an enqueue and a dequeue? (ie why would I need the same lock for enqueue and dequeue?) \$\endgroup\$ – Vaccano Feb 27 '14 at 0:04
  • 1
    \$\begingroup\$ @Vaccano The problem is that Queue (unlike ConcurrentQueue) is not documented/guaranteed to be thread-safe: see the 'Thread Safety' section at the end of msdn.microsoft.com/en-us/library/7977ey2c(v=vs.110).aspx \$\endgroup\$ – ChrisW Feb 27 '14 at 0:07

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.