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."

Answer 2

Use WhenAll instead of WaitAll, Turn your Execute into async Task and await the task returned by Task.WhenAll.

This way it never blocks on an asynchronous code.

Answer 3

I found some more detailed articles explaining why actually deadlock happened here:

https://medium.com/bynder-tech/c-why-you-should-use-configureawait-false-in-your-library-code-d7837dce3d7f

https://blog.stephencleary.com/2012/07/dont-block-on-async-code.html

Short answer would be making a small change in my async method so it looks like that:

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

Yup. That's it. Suddenly it doesn't deadlock. But these two articles + @FCin response explain WHY it actually happened.

Answer 4

It's like you are saying i do not care when ReadImagesAsync() finishes but you have to wait for it .... Here is a definition

The Task.WaitAll blocks the current thread until all other tasks have completed execution.

The Task.WhenAll method is used to create a task that will complete if and only if all the other tasks are complete.

So, if you are using Task.WhenAll you would get a task object that isn't complete. However, it will not block and would allow the program to execute. On the contrary, the Task.WaitAll method call actually blocks and waits for all other tasks to complete.

Essentially, Task.WhenAll would provide you a task that isn't complete but you can use ContinueWith as soon as the specified tasks have completed their execution. Note that neither the Task.WhenAll method nor the Task.WaitAll would run the tasks, i.e., no tasks are started by any of these methods.

Task.WhenAll(taskList).ContinueWith(t => {

  // write your code here

});