views:

88

answers:

3

I'm tinkering away on a multithreaded downloader, using a producer/consumer queue construct; the downloading parts works fine, but I'm running into a problem keeping the GUI updated.

For now I'm using a listbox control on the form to display status messages and updates on downloading progress, eventually I hope to replace that with progressbars.

First the Form1 code behind; the form contains nothing but a button and the listbox:

public partial class Form1 : Form
{
    public Form1()
    {
        InitializeComponent();
    }

    public void SetProgressMessage(string message) 
    { 
        if (listboxProgressMessages.InvokeRequired) 
        {
            listboxProgressMessages.Invoke(new MethodInvoker(delegate()
            { SetProgressMessage(message); })); 
        } 
        else 
        {
            listboxProgressMessages.Items.Add(message);
            listboxProgressMessages.Update();
        } 
    }

    private void buttonDownload_Click(object sender, EventArgs e)
    {
        SetProgressMessage("Enqueueing tasks");
        using (TaskQueue q = new TaskQueue(4))
        {
            q.EnqueueTask("url");
            q.EnqueueTask("url");
            q.EnqueueTask("url");
            q.EnqueueTask("url");
            q.EnqueueTask("url");
            q.EnqueueTask("url");
            q.EnqueueTask("url");
            q.EnqueueTask("url");
            q.EnqueueTask("url");
            q.EnqueueTask("url");
        }
        SetProgressMessage("All done!");
    }
}

Now the producer/consumer logic. The consumer downloads the files bit by bit, and should tell the listbox living on the GUI thread how the progress is coming along; This works, but the listbox doesn't actually update until all is finished, also the messages appear after the 'All done!' message, which isn't desirable.

TaskQueue.cs:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading;
using System.IO;
using System.Net;
using System.Windows.Forms;
using System.Runtime.Remoting.Messaging;

namespace WinformProducerConsumer
{
    public class TaskQueue : IDisposable
    {
        object queuelocker = new object();
        Thread[] workers;
        Queue<string> taskQ = new Queue<string>();

    public TaskQueue(int workerCount)
    {
        workers = new Thread[workerCount];
        for (int i = 0; i < workerCount; i++)
            (workers[i] = new Thread(Consume)).Start();
    }

    public void Dispose()
    {
        foreach (Thread worker in workers) EnqueueTask(null);
        foreach (Thread worker in workers) worker.Join();
    }

    public void EnqueueTask(string task)
    {
        lock (queuelocker)
        {
            taskQ.Enqueue(task);
            Monitor.PulseAll(queuelocker);
        }
    }

    void Consume()
    {
        while (true)
        {
            string task;
            Random random = new Random(1);
            lock (queuelocker)
            {
                while (taskQ.Count == 0) Monitor.Wait(queuelocker);
                task = taskQ.Dequeue();
            }
            if (task == null) return;

            try
            {
                Uri url = new Uri(task);
                HttpWebRequest request = (HttpWebRequest)WebRequest.Create(url);
                HttpWebResponse response = (HttpWebResponse)request.GetResponse();
                response.Close();
                Int64 iSize = response.ContentLength;
                Int64 iRunningByteTotal = 0;

                using (WebClient client = new System.Net.WebClient())
                {
                    using (Stream streamRemote = client.OpenRead(new Uri(task)))
                    {
                        using (Stream streamLocal = new FileStream(@"images\" + Path.GetFileName(task), FileMode.Create, FileAccess.Write, FileShare.None))
                        {
                            int iByteSize = 0;
                            byte[] byteBuffer = new byte[iSize];
                            while ((iByteSize = streamRemote.Read(byteBuffer, 0, byteBuffer.Length)) > 0)
                            {
                                streamLocal.Write(byteBuffer, 0, iByteSize);
                                iRunningByteTotal += iByteSize;

                                double dIndex = (double)iRunningByteTotal;
                                double dTotal = (double)byteBuffer.Length;
                                double dProgressPercentage = (dIndex / dTotal);
                                int iProgressPercentage = (int)(dProgressPercentage * 100);

                                string message = String.Format("Thread: {0} Done: {1}% File: {2}",
                                    Thread.CurrentThread.ManagedThreadId,
                                    iProgressPercentage,
                                    task);

                                Form1 frm1 = (Form1)FindOpenForm(typeof(Form1));
                                frm1.BeginInvoke(new MethodInvoker(delegate()
                                {
                                    frm1.SetProgressMessage(message);
                                })); 
                            }
                            streamLocal.Close();
                        }
                        streamRemote.Close();
                    }
                }

            }
            catch (Exception ex)
            {
                // Generate message for user
            }
        }
    }

    private static Form FindOpenForm(Type typ) 
    { 
        for (int i1 = 0; i1 < Application.OpenForms.Count; i1++) 
        { 
            if (!Application.OpenForms[i1].IsDisposed && (Application.OpenForms[i1].GetType() == typ))
            { 
                return Application.OpenForms[i1]; 
            } 
        } 
        return null; 
    }
}

}

Any suggestions, examples? I've looked around for solutions, but couldn't find anything that I could follow or worked.

Replacing the frm1.BeginInvoke(new MethodInvoker(delegate() with a frm1.Invoke(new MethodInvoker(delegate() results in a deadlock. I'm rather stumped here.

Sources: Producer/Consumer example: http://www.albahari.com/threading/part4.aspx

Update: I'm going about this the wrong way; instead of invoking back to the GUI from the worker threads, I'll use events that the GUI thread will have to keep an eye on. A lesson learned. :)

+1  A: 

My other answer is more appropriate. This answer is more appropriate once you change TaskQueue to raise ProgressChanged event.


Try calling listboxProgressMessages.Refresh(). This forces a paint. Check Control.Refresh documentation. Sometimes, you have to call the refresh method of the form.

Control.Refresh Method

Forces the control to invalidate its client area and immediately redraw itself and any child controls.

http://msdn.microsoft.com/en-us/library/system.windows.forms.control.refresh.aspx

AMissico
I've tried both .Refresh() and .Update(); They go off after everything is finished. In other words, nothing happens until 'All done!' appears, and then it adds the progress messages and refreshes the listbox as many times after adding each line.I think this is a timing thing; it seems that while in the Using of the producer/consumer queue the consumer can't communicate back to the gui thread.
Alkone
It is possible that the threads are not starting. (They start when the system has allocated the time. They do not start immediately.) Leave the Refresh. Put a Sleep(250) after the Refresh for debugging purposes. Put a Sleep(110) after each EnqueueTask. Make sure each task takes 5000+ms. Any timing issue will "jump" out at you. (Sleep must be over 100ms.) If this does not work, last troubleshooting step is to replace the sleep with DoEvents. If it still does not work with DoEvents, then you have something else going on that is not form related.
AMissico
The threads start; I see the files appear in /images as they should, so not worried about that.It grinds to a halt if I use Invoke (deadlock?). If I use BeginInvoke, the displaying is delayed until after all downloads are finished.
Alkone
Yes, what you are describing is expected behavior because the system only schedules the begin-invoke. Once it is free, the begin-invokes execute. See my other answer for the fix. Now that you did it the hard way, you will always use events. :O)
AMissico
A: 

Also in your code, instead of calling BeginInvoke, call Invoke - the latter is synchronous from the caller. This will mean that you call across to the GUI and wait until the GUI has finished what its doing before returning.

Also, the GUI thread could be starved, I think invokes onto the GUI thread for controls are actually marshalled onto the message pump - if the pump doesn't pump, the call never gets through.

Update: alternatively, you can use the BackgroundWorker form component. This has been built to work in WinForms, thus working with the GUI. It exposes a ProgressChanged event that you can use to pass arbitrary progress reports back to the UI. The event itself is marshalled so that it calls on the GUI thread automatically. This removes the need for you to do it manually.

BackgroundWorker threads also use the thread pool.

Adam
I considered that and tried it, but it grinds the app to a halt; probably a deadlock. Two files appear in the download directory (even with more than two worker threads assigned to consuming the queue), but they remain at 0 bytes in size. Without the Invoke/BeginInvoke/methodcall the threads do as they are supposed to.Any suggestions or alternative ways of getting information from the worker thread back to the GUI thread?
Alkone
Yes, you can use a BackgroundWorker. I'll amend my answer.
Adam
Thank you for the suggestion. :)
Alkone
+2  A: 

You should eliminate FindOpenForm and add a ProgressChanged event to TaskQueue. It is absolutely not TaskQueue responsibility to make direct calls to the presentation-layer (forms or controls). It is then the form's responsibility to listen for "progress-changed" events generated by tasks, and then update itself propertly.

This will easily solve your problem, it will keep simple, follow best-practices, and eliminate timing issues, threading issues, and so on.

AMissico
Alrighty, I will search for a solution in this direction instead. :)
Alkone
Solution is very easy. pseudo-code. public event progresschange as eventhandler. remove findopenform, replace with raise progresschange. In form, addhandler q.progresschange, addressof form_reportprogresschange. In form_reportprogresschange call SetProgressMessage.
AMissico
I am late, but work through this. It is a good programming exercise and is a fundemental .NET skill.
AMissico
Thank you kindly for the additional information and links. I've used backgroundworker and threadpool in the past, and wanted to use producer/consumer queue for this solution. (Just to do something else.. ;)
Alkone