Infinite loop while downloading multiple files with WebClient

Question

The conception: I'm making a C# app which downloads files from a given URL. Textbox, url added, file downloads, every event occurs in the correct way.

I'm trying to recreate this program to download multiple files, one by one. I have a textbox with one url/line, parsing happens correctly, I have all the links in a string array which was placed in the textbox. Then it starts downloading async, and I wanted to make it download only one by one, so I made a while loop in a foreach loop, as I don't want to go to the next url until the current one has finished downloading.

The problem is: I get in an infinite loop (though I made this work before (idk how), if I placed a messagebox in the while loop (note: I retried a minute ago, it didn't do the trick this time)).

I'll just show the code snippet:

foreach (string url in urllist)
{
    isdonwloaded = false;
    string filename = url.Split('/').Last();
    label3.Text = filename;
    webclient.DownloadFileAsync(new Uri(url), @"C:\Users\Krisz" + @"\" + filename);

    while (!isdonwloaded) // this was the first idea, but with webclient.IsBusy it did the same thing
    {
        // MessageBox.Show(counter);
        Thread.Sleep(1000);
        label8.Text = "Download in progress...";
    }

    counter++;
    label8.Text = "Done!";
}

// Events:
webclient.DownloadProgressChanged += new DownloadProgressChangedEventHandler(webc_DownloadProgressChanged);
webclient.DownloadFileCompleted += new AsyncCompletedEventHandler(webc_DownloadFileCompleted);

// The DownloadFileCompleted event:
void webc_DownloadFileCompleted(object sender, AsyncCompletedEventArgs e)
{
    label7.Text = String.Format("Files {0} / {1}", counter, arraylength(urllist));
    isdonwloaded = true;
}

I have looked into this thread: WebClient.DownloadFileAsync - Download files one at a time , but I couldn't manage to make it work either that way. (Maybe I misunderstood something?)

Can someone give me some hints, what did I do wrong? I never really used events, so it was only matter of time to run into an error. Every bit of help is greatly appreaciated, this program would be a useful one for me.

I have edited your title. Please see, "[Should questions include “tags” in their titles?](http://meta.stackexchange.com/questions/19190/)", where the consensus is "no, they should not". — John Saunders, Mar 07 '13 at 16:19
Oh, well. :D Sorry, I did mention it's my first post. Thanks for correcting, next time I'll look after these. — csakii, Mar 07 '13 at 22:16
No problem. Since you liked that one, here's another: Unlike forum sites, we don't use "Thanks", or "Any help appreciated", or signatures on [so]. See "[Should 'Hi', 'thanks,' taglines, and salutations be removed from posts?](http://meta.stackexchange.com/questions/2950/should-hi-thanks-taglines-and-salutations-be-removed-from-posts). — John Saunders, Mar 07 '13 at 22:19

Eric Lippert · Accepted Answer · 2013-03-07T23:09:28.597

OK, first off, understand why your code isn't working now.

Imagine you have two people in an office. Both have "in boxes". Their workflow is: they check their inbox. If there's a task in the inbox then they perform the task until it is finished, and then check their inbox again, repeat.

Worker 1 gets a message in their inbox that says your next task is:

turn the switch off
change the label to "downloading"
tell worker 2 to download the file
check to see if the switch is on -- if it is then break out of the loop; if not then go to sleep for one second.
go back to the previous step
change the label to "done"
this task is now finished

Worker 1 turns the switch off and puts the following task in worker 2's inbox:

download the file
tell worker 1 to turn the switch on
this task is finished

Worker 1 then checks to see if the switch is on. It is not, so worker 1 goes to sleep.

Worker 2 downloads the file, and then puts a message in worker 1's inbox that says:

turn the switch on
this task is finished

And now you see why worker 1 sleeps forever, right? That switch is never going to be flipped because it is worker 1's job to flip that switch, and worker 1 is sleeping until it is flipped. Worker 1 does not look in their inbox until the current task is finished, and the current task doesn't finish until that switch is flipped.

This gives us an idea for a solution, but it is not a good one.

The cheap, dirty, dangerous and ill-advised way to solve this problem is to use "DoEvents" instead of "Sleep". That changes the task to:

turn the switch off
change the label to "downloading"
tell worker 2 to download the file
check to see if the switch is on -- if it is then break out of the loop; if not then check your inbox for messages and do anything you find in there.
go back to the previous step
change the label to "done"
this task is finished

This solves your immediate problem but it introduces new problems. We now no longer have a clean workflow; one inbox task can spawn a second inbox task, which can in turn spawn a third inbox task. Tasks can become "re-entrant", where one task ends up starting a second version of itself. This solution is inelegant and makes for situations that are difficult to debug. Ideally you want your inbox tasks to have the property that a new one is started after the old one is finished, not while the old one still has work to do.

A better cheap-and-dirty fix for your problem (if you are using C# 5) is to use

await Task.Delay(1000);

instead of Sleep or DoEvents. That makes a subtle change to the workflow. Basically it becomes:

turn the switch off
change the label to "downloading"
tell worker 2 to download the file
check to see if the switch is on
if it is on then change the label to "done"; this task is finished.
if not then ask worker 3 to send me a task in one second; this task is finished.

If worker 3 is told to send worker 1 a new task, then the new task it sends is:

check to see if the switch is on
if it is on then change the label to "done"; this task is finished.
if not then ask worker 3 to send me a task in one second; this task is finished.

You see how that subtly but correctly changes the workflow? Now worker 1 changes the label to downloading, sends a message to worker 2, checks the switch, sends a message to worker 3, and then goes back to its inbox. Worker 2 does the download and sends a message to worker 1. Worker 1 flips the switch and goes back to the inbox. Worker 3 sends a message to worker 1. Worker 1 checks the switch, changes the label to done, and goes back to the inbox.

Now no task tells you to look in your inbox for more tasks. Each inbox task is processed in order: later-arriving tasks are always started after earlier-arriving tasks are finished.

The best solution however would be to have a version of DownloadClientAsync that could itself return a task that could be awaited. Unfortunately, it is void-returning. Building a special version of DownloadClientAsync that returns a Task that can be awaited is left as an exercise. Once you have such a helper method then the code becomes trivial; you just await that task.

Whoah, thanks for taking the time. :D I have to reread this a few times, and once I have free 3 hours, I'll sit down and convert your explanation into source code. Until this: it's weekend! — csakii, Mar 07 '13 at 22:58
Now, I have sat down a bit, trying to understand.. Could you show me how would it look like in code? Never used DoEvents nor await. I'll - of course - look after it over the internet, but having a real-time human explainer makes things a lot more easier. :) — csakii, Mar 08 '13 at 08:26

score 2 · Answer 2 · answered Mar 07 '13 at 16:42

2

Did you try shifting the event subscription code from current line to the line after :

webclient.DownloadFileAsync(new Uri(url), @"C:\Users\Krisz" + @"\" + filename);

  webclient.DownloadFileCompleted -= new AsyncCompletedEventHandler(webc_DownloadFileCompleted); 
//Also it's a good practice to unsubscribe to event once after we are out-of-scope.

 webclient.DownloadFileCompleted += new AsyncCompletedEventHandler(webc_DownloadFileCompleted);

When i did this the rest worked fine for me, otherwise the screen was blank as you told.

answered Mar 07 '13 at 16:42

Siva Gopal

3,474
1
25
22

I think it could be because of sleeping on thread that too in a loop until you are finished. It would be best if you implement Eric's suggestion if you use C# 5 (async is in built). Otherwise did you try doing the downloads on a background worker/separate Thread, because by definition 'DownloadFileAsync' should not block calling Thread. – Siva Gopal Mar 08 '13 at 08:50
So, I gotta swim a bit in this threading topic, since I usually code single thread apps. Well, a new level.. (/me launches msdn) – csakii Mar 08 '13 at 20:40

score 1 · Answer 3 · answered Mar 07 '13 at 16:41

I think this is very similar to the BackgroundWorker class, where the CompletedEventHandler runs on the main thread. (I can't find any results confirming this at the moment.)

This means that your main loop will never be interrupted - you would have to exit your main URL loop and return to the UI before webc_DownloadFileCompleted can possibly fire.

One possible fix is to run a SINGLE webclient async download on your first URL, then return to the main UI. Your webc_DownloadFileCompleted function can re-issue the next async download call.

Infinite loop while downloading multiple files with WebClient

3 Answers3

Linked