Updating GUI using Tasks [duplicate]

Question

I have a weird problem that i can't solve, i have a form that i open within another form, as soon as i open that form and since there is no event to fire after page finish loading, at the form load event i set a Timer that will start after 5sec. The timer will trigger a Task that will download files, downloading files is updated in a progressbar. The problem is that anything i try to change while Tasks are running doesn't update the GUI and will only change GUI after all Tasks finishes, note that the progressbar gets updated fine. Here is my code:

private void frm_HosterDownloader_Load(object sender, EventArgs e)
{
    StartDownloadTimer = new Timer();
    StartDownloadTimer.Tick += StartDownloadTimer_Tick;
    StartDownloadTimer.Interval = 5000;
    StartDownloadTimer.Start();
}

void StartDownloadTimer_Tick(object sender, EventArgs e)
{
    StartDownload();
    StartDownloadTimer.Stop();
}

private void StartDownload()
{
    int counter = 0;
    Dictionary<string, string> hosters = Hosters.GetAllHostersUrls();

    progressBar_Download.Maximum = hosters.Count * 100;
    progressBar_Download.Minimum = 0;
    progressBar_Download.Value = 0;

    foreach (KeyValuePair<string, string> host in hosters)
    {
        //Updating these tow lables never works, only when  everything finishes
        lbl_FileName.Text = host.Key;
        lbl_NumberOfDownloads.Text = (++counter).ToString() + "/" + hosters.Count().ToString();

        Task downloadTask = new Task(() =>
        {
            Downloader downloader = new Downloader(host.Value, string.Format(HostersImagesFolder + @"\{0}.png", IllegalChars(host.Key)));
            downloader.HosterName = host.Key;
            downloader.DownloadFinished += downloader_DownloadFinished;
            downloader.Execute();

        });
        downloadTask.Start();
        downloadTask.Wait();
    }
}

void downloader_DownloadFinished(object sender, ProgressEventArgs e)
{
    progressBar_Download.Value = progressBar_Download.Value + (int)e.ProgressPercentage;
}

I tired putting the tow label statments within the Task and even tried to pass them as an argument to be updated in the DownloadFinish event but no luck.

Edit:

Here is the Downloader Class:

 public class Downloader : DownloaderBase
{
    public string HosterName { set; get; }

    /// <summary>
    /// Initializes a new instance of the <see cref="Downloader"/> class.
    /// </summary>
    /// <param name="hoster">The hoster to download.</param>
    /// <param name="savePath">The path to save the video.</param>
    /// <param name="bytesToDownload">An optional value to limit the number of bytes to download.</param>
    /// <exception cref="ArgumentNullException"><paramref name="video"/> or <paramref name="savePath"/> is <c>null</c>.</exception>
    public Downloader(string hosterUrl, string savePath, int? bytesToDownload = null)
        : base(hosterUrl, savePath, bytesToDownload)
    { }

    /// <summary>
    /// Occurs when the downlaod progress of the file file has changed.
    /// </summary>
    public event EventHandler<ProgressEventArgs> DownloadProgressChanged;

    /// <summary>
    /// Starts download.
    /// </summary>
    /// <exception cref="IOException">The video file could not be saved.</exception>
    /// <exception cref="WebException">An error occured while downloading the video.</exception>
    public override void Execute()
    {
        this.OnDownloadStarted(new ProgressEventArgs(0, HosterName));

        var request = (HttpWebRequest)WebRequest.Create(this.HosterUrl);

        if (this.BytesToDownload.HasValue)
        {
            request.AddRange(0, this.BytesToDownload.Value - 1);
        }

        try
        {
            // the following code is alternative, you may implement the function after your needs
            request.Timeout = 100000;
            request.ReadWriteTimeout = 100000;
            request.ContinueTimeout = 100000;
            using (WebResponse response = request.GetResponse())
            {
                using (Stream source = response.GetResponseStream())
                {
                    using (FileStream target = File.Open(this.SavePath, FileMode.Create, FileAccess.Write))
                    {
                        var buffer = new byte[1024];
                        bool cancel = false;
                        int bytes;
                        int copiedBytes = 0;

                        while (!cancel && (bytes = source.Read(buffer, 0, buffer.Length)) > 0)
                        {
                            target.Write(buffer, 0, bytes);

                            copiedBytes += bytes;

                            var eventArgs = new ProgressEventArgs((copiedBytes * 1.0 / response.ContentLength) * 100, HosterName);

                            if (this.DownloadProgressChanged != null)
                            {
                                this.DownloadProgressChanged(this, eventArgs);

                                if (eventArgs.Cancel)
                                {
                                    cancel = true;
                                }
                            }
                        }
                    }
                }
            }
        }
        catch (WebException ex)
        {
            if (ex.Status == WebExceptionStatus.Timeout)
                Execute();
        }

        this.OnDownloadFinished(new ProgressEventArgs(100, HosterName));
    }
}

 public abstract class DownloaderBase
{
    /// <summary>
    /// Initializes a new instance of the <see cref="DownloaderBase"/> class.
    /// </summary>
    /// <param name="hosterUrl">The video to download/convert.</param>
    /// <param name="savePath">The path to save the video/audio.</param>
    /// /// <param name="bytesToDownload">An optional value to limit the number of bytes to download.</param>
    /// <exception cref="ArgumentNullException"><paramref name="hosterUrl"/> or <paramref name="savePath"/> is <c>null</c>.</exception>
    protected DownloaderBase(string hosterUrl, string savePath, int? bytesToDownload = null)
    {
        if (hosterUrl == null)
            throw new ArgumentNullException("video");

        if (savePath == null)
            throw new ArgumentNullException("savePath");

        this.HosterUrl = hosterUrl;
        this.SavePath = savePath;
        this.BytesToDownload = bytesToDownload;
    }

    /// <summary>
    /// Occurs when the download finished.
    /// </summary>
    public event EventHandler<ProgressEventArgs> DownloadFinished;

    /// <summary>
    /// Occurs when the download is starts.
    /// </summary>
    public event EventHandler<ProgressEventArgs> DownloadStarted;

    /// <summary>
    /// Gets the number of bytes to download. <c>null</c>, if everything is downloaded.
    /// </summary>
    public string HosterUrl { get; set; }

    /// <summary>
    /// Gets the number of bytes to download. <c>null</c>, if everything is downloaded.
    /// </summary>
    public int? BytesToDownload { get; private set; }

    /// <summary>
    /// Gets the path to save the video/audio.
    /// </summary>
    public string SavePath { get; private set; }

    /// <summary>
    /// Starts the work of the <see cref="DownloaderBase"/>.
    /// </summary>
    public abstract void Execute();

    protected void OnDownloadFinished(ProgressEventArgs e)
    {
        if (this.DownloadFinished != null)
        {
            this.DownloadFinished(this, e);
        }
    }

    protected void OnDownloadStarted(ProgressEventArgs e)
    {
        if (this.DownloadStarted != null)
        {
            this.DownloadStarted(this, e);
        }
    }
}

using the shown event didn't work for me because some controls didn't show — ykh
– ykh, Commented Aug 20, 2014 at 17:37
No reason to use a threadpool thread to do blocking I/O bound work. What is Downloader? — Yuval Itzchakov
– Yuval Itzchakov, Commented Aug 20, 2014 at 17:40

Henk Holterman · Accepted Answer · 2014-08-20 17:54:21Z

3

It is not useful to use a Task this way:

downloadTask.Start();
downloadTask.Wait();

The Wait() will block the calling code and that is handling an event. Your downloads are effectively executing on the main GUI thread, blocking it.

The solution is

//downloadTask.Wait();

You don't seem to need it.

edited Aug 20, 2014 at 17:54

answered Aug 20, 2014 at 17:33

Henk Holterman

276k33 gold badges353 silver badges540 bronze badges

Sign up to request clarification or add additional context in comments.

6 Comments

ykh Over a year ago

doesn't the Wait() will block only the task. The statements that update the controls are inside the foreach and not the Task body. Plus i already tried what you have suggested but same outcome.

Henk Holterman Over a year ago

No, it blocks the Thread that calls Wait().

ykh Over a year ago

I tried your solution, but still not updating.

Henk Holterman Over a year ago

Not updating (a Control) or totally blocked?

ykh Over a year ago

Not updating a control. It only shows final update after all tasks finishes

|

Yuval Itzchakov · Accepted Answer · 2014-08-20 19:46:22Z

2

There is rarely a good reason to use threads (either new ones you create or threadpool) to do IO bound work. Here is an async alternative to your synchronous Execute method:

public async Task ExecuteAsync()
{
    this.OnDownloadStarted(new ProgressEventArgs(0, HosterName));

    var httpClient = new HttpClient();
    var request = (HttpWebRequest)WebRequest.Create(this.HosterUrl);

    if (this.BytesToDownload.HasValue)
    {
        request.AddRange(0, this.BytesToDownload.Value - 1);
    }

    try
    {
        request.Timeout = 100000;
        request.ReadWriteTimeout = 100000;
        request.ContinueTimeout = 100000;

        var response = await httpClient.SendAsync(request);
        var responseStream = await response.Content.ReadAsStreamAsync();

        using (FileStream target = File.Open(this.SavePath, FileMode.Create, FileAccess.Write))
        {
            var buffer = new byte[1024];
            bool cancel = false;
            int bytes;
            int copiedBytes = 0;

            while (!cancel && (bytes = await responseStream.ReadAsync(buffer, 0, buffer.Length)) > 0)
            {
                await target.WriteAsync(buffer, 0, bytes);

                copiedBytes += bytes;

                var eventArgs = new ProgressEventArgs((copiedBytes * 1.0 / response.ContentLength) * 100, HosterName);

                if (this.DownloadProgressChanged != null)
                {
                    this.DownloadProgressChanged(this, eventArgs);

                    if (eventArgs.Cancel)
                    {
                        cancel = true;
                    }
                }
            }
        }
    }

    catch (WebException ex)
    {
        if (ex.Status == WebExceptionStatus.Timeout)
    }

    this.OnDownloadFinished(new ProgressEventArgs(100, HosterName));
}

Now, there is no need to use Task.Wait or create a new Task. IO bound work is asynchronous by nature. In a combination with the new async-await keywords in C# 5, you can keep your UI responsive through the whole time, as each await yields control back to the calling method, and frees your winforms message pump to process more messasges in the meanwhile.

private async void frm_HosterDownloader_Load(object sender, EventArgs e)
{
    await Task.Delay(5000);
    await StartDownloadAsync();
}

private async Task StartDownloadAsync()
{
    int counter = 0;
    Dictionary<string, string> hosters = Hosters.GetAllHostersUrls();

    progressBar_Download.Maximum = hosters.Count * 100;
    progressBar_Download.Minimum = 0;
    progressBar_Download.Value = 0;

    var downloadTasks = hosters.Select(hoster => 
    {
        lbl_FileName.Text = hoster.Key;
        lbl_NumberOfDownloads.Text = (++counter).ToString() + "/" + hosters.Count().ToString();

        Downloader downloader = new Downloader(host.Value, string.Format(HostersImagesFolder + @"\{0}.png", IllegalChars(host.Key)));
        downloader.HosterName = host.Key;
        downloader.DownloadFinished += downloader_DownloadFinished;

        return downloader.ExecuteAsync(); 
    });

    return Task.WhenAll(downloadTasks);
}

Note i changed your timer to a Task.Delay, since it internally uses a timer and you only need it to execute once.

If you want more on the use of async-await, you can start here.

edited Aug 20, 2014 at 19:46

answered Aug 20, 2014 at 18:25

Yuval Itzchakov

150k32 gold badges275 silver badges333 bronze badges

5 Comments

Servy Over a year ago

The OP's code wasn't actually ever sleeping a thread pool thread ever either, he was simply using event-based asynchrony instead of task based asynchrony. Different style, but equally valid.

Yuval Itzchakov Over a year ago

Task downloadTask = new Task, downloadTask.Start(). He was using a threadpool thread. He wasn't sleeping it, but he was definitely consuming it.

Servy Over a year ago

He was starting an asynchronous operation in a new thread. That could be simply omitted, given that the operation itself is itself asynchronous already. The entire application doesn't need to be refactored to fix that, just that one line.

Yuval Itzchakov Over a year ago

What i wanted to improve was the unnecessary use of a Task. You're right, he could of used the DownloadXXXAsync EAP pattern with his code, but he chose to use a blocking code via GetResponse for some reason. I like the use of TAP, it simplifies things.

Yuval Itzchakov Over a year ago

@Servy he wasn't using an asynchronous operation in a new threadpool thread, he was making a blocking call via GetResponse and reading the buffer synchronously into a FileStream

Collectives™ on Stack Overflow

Updating GUI using Tasks [duplicate]

2 Answers 2

6 Comments

5 Comments

Linked

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

6 Comments

5 Comments

Linked

Related