3

So I'm writing an application in which I want to expose a series of methods with both synchronous and asynchronous equivalents. To do this, I figured the easiest approach was to write the logic in the asnyc method, and write synchronous methods as wrappers around the async methods, waiting synchronously for them to deliver their results. The code isn't playing ball. In the following code sample (not my real code but a reduction of the basic problem), the line Console.WriteLine(result) is never reached - the preceding line hangs forever. Curiously though, if I copy this pattern more or less verbatim into a Console app, it works.

What am I doing wrong? Is this simply a bad pattern, and if so, what pattern should I be using instead?

public partial class MainWindow : Window {

    public MainWindow() {
        this.InitializeComponent();
        var result = MyMethod(); //Never returns
        Console.WriteLine(result);
    }

    public string MyMethod() {
        return MyMethodAsync().Result; //Hangs here
    }

    public async Task<string> MyMethodAsync() { //Imagine the logic here is more complex
        using (var cl = new HttpClient()) {
            return await cl.GetStringAsync("http://www.google.co.uk/");
        }
    }

}
Yuval Itzchakov
  • 146,575
  • 32
  • 257
  • 321
wwarby
  • 1,873
  • 1
  • 21
  • 37

3 Answers3

5

This is a classic deadlock. The UI is waiting on the async method to finish, but the async method trys to update the UI thread and BOOM, deadlock.

Curiously though, if I copy this pattern more or less verbatim into a Console app, it works.

That's because your WinForm application has a custom SynchronizationContext. It is caputred implicitly and its job is to marshal work back onto the UI thread once returning from your await.

Should you really expose synchronous wrappers around asynchronous operations?, the answer is no.

There is a way out of it, but i dont really like it. If you absolutely have to (you don't) call your code synchronously (again, you really shouldn't), use ConfigureAwait(false) inside the async method. This instructs the awaitable not to capture the current synccontext, so it wont marshal work back onto the UI thread:

public async Task<string> MyMethodAsync() 
{ 
    using (var cl = new HttpClient()) 
    {
        return await cl.GetStringAsync("http://www.google.co.uk/")
                       .ConfigureAwait(false);
    }
}

Note that if you do this and then try to call any UI element afterwards, you'll end up with an InvalidOperationException since you won't be on the UI thread.

Initializing the UI via a constructor is a common pattern. Stephan Cleary has a very nice series on async which you can find here.

What am I doing wrong? Is this simply a bad pattern, and if so, what pattern should I be using instead?

Yes, absolutely. If you want to expose both asynchronous and synchronous APIs, use the proper api's which wouldn't get you into this situation (a deadlock) in the firstcase. For example, if you want to expose a synchronous DownloadString, use WebClient instead.

Yuval Itzchakov
  • 146,575
  • 32
  • 257
  • 321
  • 1
    Thanks so much guys for the lightning fast responses - I it seems pretty clear what the consensus is here. I did consider using WebClient for this exact reason on the non-async branch but where I have long chains of async methods I don't want to have a load of duplicated methods all the way down to get at a synchronous method at the bottom. I can see how to do what I intended now with ConfigureAwait, but I think I'll follow the advice and let the operation decide - that is, not offer synchronous methods which do network-bound work at all. – wwarby Nov 15 '14 at 18:07
  • @wwarby That's great. I'd go with the same approach if i had to choose. – Yuval Itzchakov Nov 15 '14 at 18:09
  • If you really want to use this approach, take care, that every call with await in the MyMethodAsync must have a ConfigureAwait(false). AND, if the method GetStringAsync contains methods with await, they must be had ConfigureAwait as well! – Sean Stayns Oct 18 '17 at 04:22
  • @SeanStayn That's why in the answer, I said I really don't like that approach. It's *super* fragile, and generally doesn't work when you have to maintain a code base for a long time. – Yuval Itzchakov Oct 18 '17 at 08:06
  • 1
    I totally agree with your answer! I want to help programmer, which try to use this approach. ;) – Sean Stayns Oct 18 '17 at 08:08
3

This is a common mistake. MyMethodAsync captures the current synchronization context, and tries to resume on the synchronization context (i.e. on the UI thread) after the await. But the UI thread is blocked because MyMethod is synchronously waiting for MyMethodAsync to complete, so you have a deadlock.

You should usually not wait synchronously on the result of an async method. If you really have to, you can change MyMethodAsync so that it doesn't capture the synchronization context, using ConfigureAwait(false):

return await cl.GetStringAsync("http://www.google.co.uk/").ConfigureAwait(false);
Thomas Levesque
  • 286,951
  • 70
  • 623
  • 758
2

Others have explained the deadlock situation (which I go into detail on my blog).

I'll address the other part of your question:

Is this simply a bad pattern, and if so, what pattern should I be using instead?

Yes, it is a bad pattern. Instead of exposing both synchronous and asynchronous APIs, let the operation itself determine whether it should be asynchronous or synchronous. E.g., CPU-bound code is generally synchronous, while I/O-bound code is generally asynchronous.

The proper pattern is to actually not expose a synchronous API for an HTTP operation:

public async Task<string> MyMethodAsync() {
    using (var cl = new HttpClient()) {
        return await cl.GetStringAsync("http://www.google.co.uk/");
    }
}

Of course, then the question is how to initialize your UI. And the proper answer there is to synchronously initialize it into a "loading" state, and asynchronously update it to a "loaded" state. That way the UI thread is not blocked:

public partial class MainWindow : Window {
  public MainWindow() {
    this.InitializeComponent();
    var _ = InitializeAsync();
  }

  private static async Task InitializeAsync()
  {
    // TODO: error handling
    var result = await MyMethodAsync();
    Console.WriteLine(result);
  }
}

I have another blog post that addresses "asynchronous initialization" with a few various approaches.

Stephen Cleary
  • 437,863
  • 77
  • 675
  • 810