davidfowl / AspNetCoreDiagnosticScenarios

This repository has examples of broken patterns in ASP.NET Core applications
7.69k stars 740 forks source link

AsyncGuidance - Constructors and DI #63

Open mihirdilip opened 4 years ago

mihirdilip commented 4 years ago

@davidfowl, Constructors section explains getting around by using static factory pattern.

But, what happens when I cannot call this static method to create an instance because the instance is created by container when IService is dependency injected via constructor into other class?

Basically, what is the best practice for calling Async method in within constructor for these 2 different scenarios?

epignosisx commented 4 years ago

One option is to delay initialization until method invocation occurs. Basically:

public async Task FooAsync()
{
  await EnsureInitializedAsync();
  //do actual operation
}

The RedisCache class is a good example of this pattern. It does not connect to Redis until the first interaction with the server: https://github.com/dotnet/extensions/blob/master/src/Caching/StackExchangeRedis/src/RedisCache.cs#L272

mihirdilip commented 4 years ago

Thanks for your reply @epignosisx, Your suggestion does help with ensuring if the initialization initiated by Fire and Forget in Constructor is completed before utilizing it. Which certainly helps with Fire and Forget initialization.

What if I need to call Async method and the value retured is used by next statement in within Contructor? UI is depended on this so has to happen within Constructor.

May be not the best example..

public interface IFoo { }
public class Foo : IFoo
{
    // Instance will be created from Container (IServiceProvider) 
    // and so cannot use static factory pattern
    public Foo(IService service)
    {
         // Only have Async method on the API
         // Is this the best right way or there is a better way of achieving the same?
         var a = service.BarAsync().GetAwaiter().GetResult();   

         DoSomethingWith(a);    // Now part of UI may be depended on value a
         DoSomethingElseWith(a);    // And another part of UI may be depended on value a
    }
}
epignosisx commented 4 years ago

The option I brought up was about moving the constructor async logic to a class method upon invocation.

I’m struggling to understand why this logic must be in the constructor. When you say UI, which technology are you referring to? Razor View? WPF/UWP Xaml?

mihirdilip commented 4 years ago

@epignosisx, may be it is just a bad example or may be I need to rephrase it. It is not a problem I am currenly having but, in case if I do come across this problem what do I do. UI could be WPF/UWP/Xamarin.Forms Xaml.

To rephrase my query, if you forget about this Async call in Constructor for now. What is the best practice to call Async method synchronously because I do not have any synchronous method on am API I may be using? It could be within constructor or a method or (in worst case) a property.

What shall I use or what is the safest to use on WPF/UWP/Xamarin.Forms and ASP.NET Core?

public interface IFoo { }
public class Foo : IFoo
{
    public Foo(IService service)
    {
         var a = service.BarAsync().GetAwaiter().GetResult();   
    }

    public void Bar()
    {
         var a = service.BarAsync().GetAwaiter().GetResult();   
    }
}
public void Bar()
{
     // Task may return result or not depending on the requirement so this can be ignored.

     // Is this the best (right?) way or there is a better way of achieving the same?
     service.BarAsync().GetAwaiter().GetResult();

     // The same can be achieved in many different ways. 
     service.BarAsync().RunSynchronously();
     service.BarAsync().Wait();
     service.BarAsync().Result;
}
kapsiR commented 4 years ago

@mihirdilip Generally speaking, you should avoid sync over async wherever possible.

The section Avoid using Task.Result and Task.Wait has more details, why you should avoid it. (thread-pool starvation, deadlocks, ...)

If you really have to do some sync over async, there are many different examples with explanations in the Deadlocks-section.

As an example, that could be a small Utility class for your needs, still not recommended to use sync over async:

    public static class AsyncUtility
    {
        /// <summary>
        /// Executes an async operation synchronously without risking a deadlock.
        /// </summary>
        /// <typeparam name="T">The result type of the operation.</typeparam>
        /// <param name="asyncOperation">The async operation.</param>
        /// <returns>The operation result.</returns>
        public static T ExecuteSynchronously<T>(Func<Task<T>> asyncOperation)
        {
            return Task.Run(asyncOperation).Result;
        }

        /// <summary>
        /// Executes an async operation synchronously without risking a deadlock.
        /// </summary>
        /// <param name="asyncOperation">The async operation.</param>
        public static void ExecuteSynchronously(Func<Task> asyncOperation)
        {
            Task.Run(asyncOperation).Wait();
        }
    }
mihirdilip commented 4 years ago

@epignosisx and @kapsiR Thank you very much for your help and guidance. Much appreciated..

davidfowl commented 3 years ago

If there's no way to avoid it then you're in the bad situation described in the document. There's no workaround for needing to block using Task.Wait or Task.Result, nor is there a good way to do it.