Task.WaitAll deadlock

Question

I have a question regarding Task.WaitAll. At first I tried to use async/await to get something like this:

private async Task ReadImagesAsync(string HTMLtag)
{
    await Task.Run(() =>
    {
        ReadImages(HTMLtag);
    });
}

Content of this function doesn't matter, it works synchronously and is completely independent from outside world.

I use it like this:

private void Execute()
{
    string tags = ConfigurationManager.AppSettings["HTMLTags"];

    var cursor = Mouse.OverrideCursor;
    Mouse.OverrideCursor = System.Windows.Input.Cursors.Wait;
    List<Task> tasks = new List<Task>();
    foreach (string tag in tags.Split(';'))
    {
         tasks.Add(ReadImagesAsync(tag));
         //tasks.Add(Task.Run(() => ReadImages(tag)));
    }

    Task.WaitAll(tasks.ToArray());
    Mouse.OverrideCursor = cursor;
}

Unfortunately I get deadlock on Task.WaitAll if I use it that way (with async/await). My functions do their jobs (so they are executed properly), but Task.WaitAll just stays here forever because apparently ReadImagesAsync doesn't return to the caller.

The commented line is approach that actually works correctly. If I comment the tasks.Add(ReadImagesAsync(tag)); line and use tasks.Add(Task.Run(() => ReadImages(tag))); - everything works well.

What am I missing here?

ReadImages method looks like that:

private void ReadImages (string HTMLtag)
{
    string section = HTMLtag.Split(':')[0];
    string tag = HTMLtag.Split(':')[1];

    List<string> UsedAdresses = new List<string>();
    var webClient = new WebClient();
    string page = webClient.DownloadString(Link);

    var siteParsed = Link.Split('/');

    string site = $"{siteParsed[0]} + // + {siteParsed[1]} + {siteParsed[2]}";

    int.TryParse(MinHeight, out int minHeight);
    int.TryParse(MinWidth, out int minWidth);

    int index = 0;

    while (index < page.Length)
    {
        int startSection = page.IndexOf("<" + section, index);
        if (startSection < 0)
            break;

        int endSection = page.IndexOf(">", startSection) + 1;
        index = endSection;

        string imgSection = page.Substring(startSection, endSection - startSection);

        int imgLinkStart = imgSection.IndexOf(tag + "=\"") + tag.Length + 2;
        if (imgLinkStart < 0 || imgLinkStart > imgSection.Length)
            continue;

        int imgLinkEnd = imgSection.IndexOf("\"", imgLinkStart);
        if (imgLinkEnd < 0)
            continue;

        string imgAdress = imgSection.Substring(imgLinkStart, imgLinkEnd - imgLinkStart);

        string format = null;
        foreach (var imgFormat in ConfigurationManager.AppSettings["ImgFormats"].Split(';'))
        {
            if (imgAdress.IndexOf(imgFormat) > 0)
            {
                format = imgFormat;
                break;
            }
        }

        // not an image
        if (format == null)
            continue;

        // some internal resource, but we can try to get it anyways
        if (!imgAdress.StartsWith("http"))
            imgAdress = site + imgAdress;

        string imgName = imgAdress.Split('/').Last();

        if (!UsedAdresses.Contains(imgAdress))
        {
            try
            {
                Bitmap pic = new Bitmap(webClient.OpenRead(imgAdress));
                if (pic.Width > minHeight && pic.Height > minWidth)
                    webClient.DownloadFile(imgAdress, SaveAdress + "\\" + imgName);
            }
            catch { }
            finally
            {
                UsedAdresses.Add(imgAdress);
            }
        }

    }
}

Answer 1

You are synchronously waiting for tasks to finish. This is not gonna work for WPF without a little bit of ConfigureAwait(false) magic. Here is a better solution:

private async Task Execute()
{
    string tags = ConfigurationManager.AppSettings["HTMLTags"];

    var cursor = Mouse.OverrideCursor;
    Mouse.OverrideCursor = System.Windows.Input.Cursors.Wait;
    List<Task> tasks = new List<Task>();
    foreach (string tag in tags.Split(';'))
    {
         tasks.Add(ReadImagesAsync(tag));
         //tasks.Add(Task.Run(() => ReadImages(tag)));
    }

    await Task.WhenAll(tasks.ToArray());
    Mouse.OverrideCursor = cursor;
}

If this is WPF, then I'm sure you would call it when some kind of event happens. The way you should call this method is from event handler, e.g.:

private async void OnWindowOpened(object sender, EventArgs args)
{
    await Execute();
}

Looking at the edited version of your question I can see that in fact you can make it all very nice and pretty by using async version of DownloadStringAsync:

private async Task ReadImages (string HTMLtag)
{
    string section = HTMLtag.Split(':')[0];
    string tag = HTMLtag.Split(':')[1];

    List<string> UsedAdresses = new List<string>();
    var webClient = new WebClient();
    string page = await webClient.DownloadStringAsync(Link);

    //...
}

Now, what's the deal with tasks.Add(Task.Run(() => ReadImages(tag)));?

This requires knowledge of SynchronizationContext. When you create a task, you copy the state of thread that scheduled the task, so you can come back to it when you are finished with await. When you call method without Task.Run, you say "I want to come back to UI thread". This is not possible, because UI thread is already waiting for the task and so they are both waiting for themselves. When you add another task to the mix, you are saying: "UI thread must schedule an 'outer' task that will schedule another, 'inner' task, that I will come back to."