aliostad / CacheCow

An implementation of HTTP Caching in .NET Core and 4.5.2+ for both the client and the server
MIT License
847 stars 172 forks source link

Deadlock fix #209

Closed DiverDW closed 6 years ago

DiverDW commented 6 years ago

Fix deadlock on client side in applications that use UI SynchronizationContext (eg WPF, WinForms, ASP.NET (not net core)).

Example application with deadlock: CacheCowDeadlockTest.zip

Fix: Use ConfigureAwait(false) for all awaited async methods. More information: https://blog.stephencleary.com/2012/07/dont-block-on-async-code.html

aliostad commented 6 years ago

OK, this created so much mayhem... :(

Your repro has the problem not the code. I panicked but anyhow, you should modify your client code and use async/await:

        private async void Button_OnClick(object sender, RoutedEventArgs e)
        {
            button.IsEnabled = false;

            try
            {
                var httpResponseMessage = await _httpClient.GetAsync("https://www.google.com/");
                var content = await httpResponseMessage.Content.ReadAsStringAsync();

                MessageBox.Show(this, "Content length: " + content.Length);
            }
            finally
            {
                button.IsEnabled = true;
            }
        }

The code above does not result in deadlock and no need for ConfigureAwait.

Problem with your repro code was that it actually uses .Result (GetAwaiter().GetResult() is the same) which should not be done. You should change the event handler to async void (which is the only time it is OK to use async in a method not returning Task and is void) and use await.

What is done is done, I might remove the ConfigureAwait(false) statements later on.

DiverDW commented 6 years ago

Yes, you are right. But I'm using your library in legacy project. Synchronous code present in many places. Rewrite all code to async – very difficult. I use ConfigureAwait(false) in many places. It solves this problem. Many third-party libraries use ConfigureAwait(false) in their code. Examples: Newtonsoft.Json, Refit, Microsoft corefx

What is the problem using ConfigureAwait(false)?

aliostad commented 6 years ago

The problem is that the context is used to flow CorrelationId which is essential in telemetry. CacheCow by providing middleware code, has to flow the correlationId since it sits between two consumer code. I will be removing ConfigureAwait in the next version.

I am sorry but forcing a change upon a library used by many consumers since you cannot fix your legacy problems does not seem right.

DiverDW commented 6 years ago

OK.

I use workaround for my legacy project:

public static class AsyncHelper
{
    public static ThreadPoolRedirector RedirectToThreadPool()
    {
        return new ThreadPoolRedirector();
    }
}

public struct ThreadPoolRedirector : INotifyCompletion
{
    #region awaiter
    public ThreadPoolRedirector GetAwaiter()
    {
        // combined awaiter and awaitable
        return this;
    }
    #endregion

    #region awaitable
    public bool IsCompleted
    {
        get
        {
            // true means execute continuation inline
            return Thread.CurrentThread.IsThreadPoolThread;
        }
    }

    public void OnCompleted(Action continuation)
    {
        ThreadPool.QueueUserWorkItem(o => continuation());
    }

    public void GetResult() { }
    #endregion
}

Using:

await AsyncHelper.RedirectToThreadPool();
// all other code is run in the thread-pool thread regardless of ConfigureAwait