How to avoid Deadlock when using singleton Http Client in Winforms application

Question

I have a legacy Windows Forms application that I am working on, I made some changes to the http client, I wanted to make it a singleton so that it could be reused throughout the application. It seems to be causing a deadlock.

I am going to paste all the code that I believe is involved below:

This is the calling code where the UI gets frozen, it never unfreezes.

private async void lbGroup_SelectedIndexChanged_1(object sender, EventArgs e)
{
    int groupId = this.lbGroup.SelectedIndex + 1;
    await LoadStores(groupId);

    //The code below freezes the application
    this.lbStore.DataSource = _stores;
    
    this.txtSearch.Enabled = true;
    this.lbStore.Enabled = true;
}

This is the LoadStores Method where the httpClient is used:

private async Task LoadStores(int group)
{
    try
    {
        HttpResponseMessage res = await _httpClient.GetAsync("api/GetStoresByGroup/" + group.ToString());
        res.EnsureSuccessStatusCode();
        if (res.IsSuccessStatusCode)
        {
            var serializedStores = await res.Content.ReadAsStringAsync();
            _stores = JsonConvert.DeserializeObject<IEnumerable<Store>>(serializedStores).Select(s => s.StoreName).ToList();
            res.Content.Dispose();
        }
    }
    catch (Exception ex)
    {
        ErrorLogger.LogError("Installation", $"Error getting stores list: {ex.Message}");
    }
}

This is the Http Singleton Class:

public static class HttpClientSingleton
{
    private static readonly HttpClient _instance;

    static HttpClientSingleton()
    {
        _instance = new HttpClient();
        _instance.BaseAddress = new Uri("https://www.i-city.co.za/");
        _instance.DefaultRequestHeaders.Accept.Clear();
        _instance.DefaultRequestHeaders.Accept.Add(new System.Net.Http.Headers.MediaTypeWithQualityHeaderValue("application/json"));
    }

    public static HttpClient Instance
    {
        get
        {
            return _instance;
        }
    }
}

This is the form constructor where the HttpClient gets initiliazed:

public partial class frmInstallationHelper : Form
{
    private static string _configDir;
    private static string _localConfigDir;
    private static int _storeID;
    private static Activation _activation;
    private static HttpClient _httpClient = HttpClientSingleton.Instance;
    private static IEnumerable<string> _stores;
    private static IEnumerable<string> _franchisees;
    private int _smsCounter;

If I wrap the http request in a using statement inside of the LoadStores method, the app runs fine, but I don't want to dispose of the http Client as that defeats the purpose of making it a singleton.

Update: Problem Found

After following @MongZhu's lead I replicated the program and confirmed that none of the above code was actually causing the deadlock. It was caused by another method that was triggered by the lbStore list Box onSelectChange event displayd below:

private void lbStore_SelectedIndexChanged_1(object sender, EventArgs e)
    {
        string store = this.lbStore.GetItemText(this.lbStore.SelectedItem);
        LoadFranchisees(store).Wait();
        this.lbFranchisees.DataSource = _franchisees;
    }

The way I solved the problem was by changing it to look as follows:

private async void lbStore_SelectedIndexChanged_1(object sender, EventArgs e)
    {
         string store = this.lbStore.GetItemText(this.lbStore.SelectedItem);
         await LoadFranchisees(store);
         this.lbFranchisees.DataSource = _franchisees;
    }

I was busy changing all the .wait() methods to async / await, and I must have forgotten this one.

Answer 1

The deadlock arises because you used Wait in a method which was triggered by an async opertaion. Unfortunately it was masked very good by the apparent hanging in the line of the initialization of the DataSource. But this initialization triggered the SelectedIndexChanged of the listbox which had the evil Wait call in it. Making this method async and await the result will evaporate the deadlock.

private async void lbStore_SelectedIndexChanged_1(object sender, EventArgs e)
{
    string store = this.lbStore.GetItemText(this.lbStore.SelectedItem);
    _franchisees = await LoadFranchisees(store);
    this.lbFranchisees.DataSource = _franchisees;
}

I would suggest to return the stores directly from the method instead of using a class variable as transmitter. This way you would also avoid race conditions (to which methods that use class variables are very much prone) If you need it further you could store the returning value inside the _stores variable. But a loading method should rather return the results instead of secretely storing it somewhere hidden from the user of this method.

private async Task<List<Store>> LoadStores(int group)
{
    try
    {
        HttpResponseMessage res = await _httpClient.GetAsync("api/GetStoresByGroup/" + group.ToString()))
        res.EnsureSuccessStatusCode();
        if (res.IsSuccessStatusCode)
        {
            var serializedStores = await res.Content.ReadAsStringAsync();
            res.Content.Dispose();
            return JsonConvert.DeserializeObject<IEnumerable<Store>>(serializedStores).Select(s => s.StoreName).ToList();

        }
    }
    catch (Exception ex)
    {
        ErrorLogger.LogError("Installation", $"Error getting stores list: {ex.Message}");
    }
}

You can await the result in the event:

private async void lbGroup_SelectedIndexChanged_1(object sender, EventArgs e)
{
    int groupId = this.lbGroup.SelectedIndex + 1;
    _stores = await LoadStores(groupId);
    this.lbStore.DataSource = _stores;
    
    this.txtSearch.Enabled = true;
    this.lbStore.Enabled = true;
}

The same logic applies to the LoadFranchisees method, refactor it so that it returns the data. This makes your code much more understandable. Don't hide information from the reader of a method. It could be you in 6 Month trying to figure out what da heck you did there.... Be nice to your future self at least ;)