BackgroundWorker implementation on Compact Framework

Question

I was looking at the code within the OpenNETCF SDF BackgroundWorker implementation for the .NET compact framework and it looks like the following code is not thread-safe. But the Smart Device Framework has been around for ages, so what am I missing? Is this thread-safe and if so, why?

Note that I'm not inclined to post the whole class becasue, even though the SDF is free to use, the expectation is that customers pay for licenses to the SDF. If any of the SDF team object to even this educational excerpt then I will pull the question immediately.

This is the background thread that dequeues method invocations to invoke them on the UI thread:

private void ProgressDispatcherProc()
{
    this.m_stopThreads = false;
    while (!this.m_stopThreads)
    {
        while (this.m_progressQueue.Count > 0)
        {
            MethodInvoker method = null;
            ProgressChangedEventArgs args = this.m_progressQueue.Dequeue();
            if (this.ProgressChanged != null)
            {
                if (method == null)
                {
                    method = () => this.ProgressChanged(this, args);
                }
                this.m_guiMarshaller.BeginInvoke(method);
                Application.DoEvents();
            }
        }
        Thread.Sleep(this.WorkerReportsProgress ? 5 : 0x3e8);
    }
}

The variable m_progressQueue is a standard System.Collections.Generic.Queue<>.

My concern is there is no locking to protect the queue, which is enqueued in one thread and dequeued in this one. The looping on a simple boolean done in while (!this.m_stopThreads) I assume is safe enough because, as I understand it, in the .NET Compact Framework, all variable accesses treated as volatile.

I've got no qualms about publishing this to further understanding of how code works. That's why we do what we do. — ctacke, Sep 07 '12 at 13:26

score 2 · Accepted Answer · answered Sep 07 '12 at 13:31

2

I'd agree that it should have a lock, at least around the Dequeue call, something like this:

lock(m_progressQueue.SyncRoot)
{
    ProgressChangedEventArgs args = this.m_progressQueue.Dequeue(); 
}

And it could probably use them in the rest of the class.

answered Sep 07 '12 at 13:31

ctacke

66,480
18
94
155

Hi Chris. I wrote a *BackgroundWorker-ish* routine a while back, but I certainly never used a Queue of `ProgressChangedEventArgs`. I'm looking at this and trying to understand the logic behind it. Could you provide some info on why it is done this way? – Sep 07 '12 at 14:05
The queue allows ReportProgress to do 2 things: 1) marshal the progress event to the UI thread and 2) *not* make the caller block while all progress handlers are doing stuff. – ctacke Sep 07 '12 at 14:36
Thanks for the answer. Follow-up, I'm curious about the Application.DoEvents. That should not strictly be needed since this thread is not blocking the UI thread? Plus, I've always wondered if you could pump the UI msg pump from an external thread...I've always assumed it would ignore calls from the non-UI thread. – tcarvin Sep 07 '12 at 19:12
DoEvents should not be needed. Don't call it from a background thread (it will either be ignored or die, not sure). – ctacke Sep 07 '12 at 20:41

BackgroundWorker implementation on Compact Framework

1 Answers1