Recursive WinRT Async Issues

Question

I have some code that does something like this:

abstract class Data
{
    Data(string name, bool load) { if (load) { Load().Wait(); }
    abstract Task Load();
}

class XmlData : Data
{
    XmlData(string name, bool load = true) : base(name, load) {}
    override async Task Load()
    {
        var file = await GetFileAsync(...);
        var xmlDoc = await LoadXmlDocAsync(file);
        ProcessXml(xmlDoc);
    }
    void ProcessXml(XmlDocument xmlDoc)
    {
        foreach (var element in xmlDoc.Nodes)
        {
            if (element.NodeName == "something")
                new XmlData(element.NodeText);
        }
    }
}

I seem to (sometimes) get wierd timing issues, where it ends up hanging the code at GetFileAsync(...). Is this caused by the recursive nature of the calls? When I change all the await calls to actually do a .Wait() for them to finish, and essentially get rid of all the asynchronous nature of the calls, my code executes fine.

When I attach the debugger and break, it is just sitting, waiting to for Load.Wait(). I don't have it with me right now. But as far as I remember, nothing else was executing. It was just waiting for the Load, but nothing else seemed to be going on. — Nick Banks, Mar 06 '12 at 19:30
@gamernb If that's the case, this is almost guaranteed a dead lock from waiting on the captured context. See my answer. — Reed Copsey, Mar 06 '12 at 19:31
Calling a virtual method in a constructor is generally a bad idea (see http://stackoverflow.com/a/448272/224370 for example) — Ian Mercer, Mar 06 '12 at 19:54

Reed Copsey · Accepted Answer · 2012-03-06T19:59:26.720

Is this caused by the recursive nature of the calls? When I change all the await calls to actually do a .Wait() for them to finish, and essentially get rid of all the asynchronous nature of the calls, my code executes fine.

It really depends -

The most likely culprit would be if your caller is somehow blocking the user interface thread (via a call to Wait(), etc). In this case, the default behavior of await is to capture the calling synchronization context, and post the results back on that context.

However, if a caller is using that context, you can get a deadlock.

This is very likely the case, and being caused by this line of code:

Data(string name, bool load) { if (load) { Load.Wait(); }

This can be easily avoided by making your library code (like this XmlData class) explicitly not use the calling synchronization context. This is typically only required for user interface code. By avoiding the capture, you do two things. First, you improve teh overall performance (often dramatically), and second, you avoid this dead lock condition.

This can be done by using ConfigureAwait and changing your code like so:

override async Task Load()
{
    var file = await GetFileAsync.(...).ConfigureAwait(false);
    var xmlDoc = await LoadXmlDocAsync(file).ConfigureAwait(false);
    ProcessXml(xmlDoc);
}

That being said - I would rethink this design a bit. There are really two problems here.

First, your putting a virtual method call in your constructor, which is fairly dangerous, and should be avoided when possible, as it can lead to unusual problems.

Second, your turning your entire asynchronous operation into a synchronous operation by putting this in the constructor with the block. I would, instead, recommend rethinking this entire thing.

Perhaps you could rework this to make some form of factory, which returns your data loaded asynchronously? This could be as simple as making the public api for creation a factory method that returns Task<Data>, or even a generic public async Task<TData> Create<TData>(string name) where TData : Data method, which would allow you to keep the construction and loading asynchronous and avoid the blocking entirely.

I think it's enough to call `ConfigureAwait()` on the first `Task`, because the second line will then execute with no synchronization context. But doing it like this might be more robust (e.g. if I decide to execute the first operation synchronously later on). — svick, Mar 06 '12 at 19:58
@svick True - mostly. I prefer this, as it's easier if you introduce anything in between the two that might change that behavior... Also, if `GetFileAsync` has already completed and runs synchronously (ie: if it's using a cached task), then the second `ConfigureAwait` will still be necessary. — Reed Copsey, Mar 06 '12 at 20:01
@svick That's just because there's no guarantee that the first task will change contexts, as it can always complete synchronously and never switch state. — Reed Copsey, Mar 06 '12 at 20:02
I got a chance to do a quick test to call ConfigureAwait(false) on all my awaits in that call path. It didn't seem to make a difference. I didn't get a chance to debug any more though. — Nick Banks, Mar 07 '12 at 17:34

Recursive WinRT Async Issues

1 Answers1