BackgroundWorker and ConcurrentQueue

Question

I have a FileSystemWatcher which is looking for new files, putting the filenames in a Queue. In a seperate Thread the Queue is worked off. My code is working, but I question if there could be information lost, because of the asynchronous process. Please watch the code explained by comments: (I think maybe I need something like a thread lock somewhere?) (Code is simplified)

public class FileOperatorAsync
{
  private ConcurrentQueue<string> fileQueue;
  private BackgroundWorker worker;
  private string inputPath;

  public FileOperatorAsync(string inputPath)
  {
     this.inputPath = inputPath;
     fileQueue = new ConcurrentQueue<string>();
     worker = new BackgroundWorker();
     worker.WorkerSupportsCancellation = true;
     worker.DoWork += worker_DoWork;
     Start();
  }

  void worker_DoWork(object sender, DoWorkEventArgs e)
  {
     try
     {
        string file;
        while (!worker.CancellationPending && fileQueue.TryDequeue(out file)) //As long as queue has files
        {
          //Do hard work with file
        }
        //Thread lock here?
        //If now Filenames get queued (Method Execute -> Worker is still busy), they wont get recognized.. or?
     }
     catch (Exception ex)
     {
        //Logging
     }
     finally
     {
        e.Cancel = true;
     }
  }

  public void Execute(string file) //called by the FileSystemWatcher
  {
     fileQueue.Enqueue(file);
     Start(); //Start only if worker is not busy
  }

  public void Start()
  {
     if (!worker.IsBusy)
        worker.RunWorkerAsync();
  }

  public void Stop()
  {
     worker.CancelAsync();
  }

}

Please, do not include a languge tag in a title unless it wouldn't made sense without it. Tags serve this purpose. — Ondrej Janacek, Feb 21 '14 at 09:35

score 2 · Accepted Answer · edited May 23 '17 at 10:26

Yes, you may have a problem with Execute. It can leave a file unprocessed by your worker.

You can solve it in two ways:
1) Your worker doesn't finish after processing all queued files. It waits on a AutoResetEvent for next file to process. In that case Execute should notify the worker by calling AutoResetEvent.Set.
Example:

AutoResetEvent event;
...
// in worker_DoWork
while(!worker.CancellationPending){
    event.WaitOne();
    // Dequeue and process all queued files
}

...
// in Execute
fileQueue.Enqueue(file);
event.Set();

2) Your worker finishes after processing all queued files (as you do now) but you can check in BackgroundWorker.RunWorkerCompleted whether there are still files to process and run the worker once again.
In that case if Execute hasn't started the worker because it was busy then the worker will be started again in BackgroundWorker.RunWorkerCompleted and the pending file will be processed.

// in  worker_RunWorkerCompleted
if (!fileQueue.IsEmpty())
    Start();

Note: if you decide to use BackgroundWorker.RunWorkerCompleted in a non-GUI application then you should be careful in Start because BackgroundWorker.RunWorkerCompleted can be invoked not on the thread where you call Execute and a race condition will occur in Start. More info: BackgroundWorker.RunWorkerCompleted and threading

if you call Start() from two different threads simultaneously then they both can see that worker.IsBusy == false and they both will call worker.RunWorkerAsync(). The thread which calls worker.RunWorkerAsync() a little later than another thread will throw an InvalidOperationException. So you should catch that exception or wrap IsBusy+RunWorkerAsync into a critical section with a lock to avoid the race condition and the exception throwing.

Thanks! I used your second solution. Yes it is a non-GUI application. Thanks for the Note! "Execute" is called by the FileSystemWatcher Created Event, so its already executed by different Threads, but its still synchronous? — JDeuker, Feb 21 '14 at 10:18
@J.D. I've added one more note to my answer to address your question. I hope I understood your question correctly. — Alex Antonov, Feb 21 '14 at 10:30

BartoszKP · Answer 2 · 2014-02-21T08:45:27.167

1

To not to have to worry about the problem, when the queue is empty and Start is called before the worker exits, you can try not leaving the worker method at all:

while (!worker.CancellationPending)
{
    while (!worker.CancellationPending && !fileQueue.TryDequeue(out file))
    {
        Thread.Sleep(2000);
    }

    if (worker.CancellationPending)
    {
        break;
    }
    //
}

Other possibility, without the inelegant sleeping would be to use the ManualResetEvent class to signal when the queue is empty and stops being empty.

edited Feb 21 '14 at 08:45

answered Feb 21 '14 at 08:19

BartoszKP

34,786
15
102
130

The Problem is not the Queue itself. The Queue is working fine and stable. I am worrying about items which are enqueued after the dequeuing is finished. Please look at the spot where I put the "//Thread lock here?" Comment – JDeuker Feb 21 '14 at 08:22
@J.D. It's unclear what do you want to achieve. There is no code there, so obviously no lock is needed. As for adding items - `Enqueue` method is also synchronized. – BartoszKP Feb 21 '14 at 08:26
Yes, there is not that much code afterwards, but I think there could be still milliseconds passing... I just want to make it as safe as possible. – JDeuker Feb 21 '14 at 08:30
@J.D. But why would you want to `lock`? What operations exactly are you trying to synchronize? – BartoszKP Feb 21 '14 at 08:34
On "//Thread lock here?" I want the Execute() Method to wait, till worker_DoWork is finished, so worker.IsBusy = false – JDeuker Feb 21 '14 at 08:36
@J.D. Ok, now I understand your concern. See my edit. – BartoszKP Feb 21 '14 at 08:45
I implemented your sleep solution first, but without the sleep, which lead to 100% CPU usage :D So I tried to improve it without an infinity loop. I will take a look at the ManualResetEvent. Thanks! – JDeuker Feb 21 '14 at 09:09

BackgroundWorker and ConcurrentQueue

2 Answers2