simpleinjector / SimpleInjector

An easy, flexible, and fast Dependency Injection library that promotes best practice to steer developers towards the pit of success.
https://simpleinjector.org
MIT License
1.21k stars 154 forks source link

Proposal for AsyncWebRequestLifestyle. #593

Closed malylemire1 closed 5 years ago

malylemire1 commented 6 years ago

Hi,

I have made and AsyncWebRequestLifestyle to resolve the problem with sub thread in a request. This is not the same as having an hybrid lifestyle has it keep the current scope in sub thread of a request. Here is the proposal

public sealed class AsyncWebRequestLifestyle : AsyncScopedLifestyle
{
    private static readonly object ScopeKey = new object();

    static public bool InRequest => HttpContext.Current != null || asyncScope != null;

    static private Scope currentScope =>
        HttpContext.Current != null ? (Scope)HttpContext.Current.Items[ScopeKey] : asyncScope;

    static private Scope asyncScope;

    public AsyncWebRequestLifestyle() : base() { }

    internal static void CleanUpWebRequest(HttpContext context)
    {
        var scope = currentScope;

        if (scope != null)
        {
            scope.Dispose();
            asyncScope = null;
        }
    }

    protected override Scope GetCurrentScopeCore(Container container)
        => GetOrCreateScope(container);

    protected override Func<Scope> CreateCurrentScopeProvider(Container container)
    {
        return () => GetOrCreateScope(container);
    }

    private static Scope GetOrCreateScope(Container container)
    {
        Scope scope = currentScope;

        if (scope == null)
        {
            scope = BeginScope(container);

            HttpContext context = HttpContext.Current;
            if (context != null)
            {
                context.Items[ScopeKey] = scope;
            }

            asyncScope = scope;
        }

        return scope;
    }
}

You can use it in a an hybrid lifestyle with execution scope.

GetExecutionScopeWebRequest = (container) => Lifestyle.CreateHybrid(
    lifestyleSelector: () => AsyncWebRequestLifestyle.InRequest
        && executionScope.GetCurrentScope(container) == null,
    trueLifestyle: webRequestScope,
    falseLifestyle: executionScope);
dotnetjunkie commented 6 years ago

Can you elaborate whats "the problem with sub thread in a request". What is the actual problem you are trying to solve?

malylemire1 commented 6 years ago

Hi,

Let me elaborate. I have some Task.Run inside a request from a class library with ConfigureAwait(false) that won't use current synchronization context and will have no HttpContext.Current. Even with solution like this one : https://blogs.msdn.microsoft.com/friis/2017/12/05/asp-net-httpcontext-in-asyncawait-patterns-using-the-task-parallel-library-part-3/

await Task.Factory.StartNew(myBackgroundDelegate(),
    CancellationToken.None,
    TaskCreationOptions.RunContinuationsAsynchronously,
    TaskScheduler.FromCurrentSynchronizationContext());

This will flow the HttpContext to the started task, but if any parent await is called with ConfigureAwait(false), the synchronization context will not be resumed and the HttpContext will be lost.

I need nested tasks to use the same scope to prevent instance duplicates.

Thanks

malylemire1 commented 6 years ago

Hi,

Here is a version without static reference to Scope. The HttpContext is never null in CleanUpWebRequest. The ConfigureAwait(false) should never be used in controller Task call. But this implementation prevent problems with class library and ConfigureAwait(false).

public sealed class AsyncWebRequestLifestyle : AsyncScopedLifestyle
{
    private static readonly object ScopeKey = new object();

    public AsyncWebRequestLifestyle() : base() { }

    public static bool InRequest(Container container, AsyncWebRequestLifestyle lifeStyle)
    {
        return HttpContext.Current != null || lifeStyle.GetCurrentScope(container) != null;
    }

    internal static void CleanUpWebRequest(HttpContext context)
    {
        Scope scope = (Scope)context.Items[ScopeKey];

        if (scope != null)
        {
            scope.Dispose();
        }
    }

    protected override Scope GetCurrentScopeCore(Container container) 
        => GetOrCreateScope(container);

    protected override Func<Scope> CreateCurrentScopeProvider(Container container)
    {
        return () => GetOrCreateScope(container);
    }

    private Scope GetOrCreateScope(Container container)
    {
        Scope scope = GetCurrentAsyncScope(container);

        if (scope == null)
        {
            scope = BeginScope(container);

            HttpContext context = HttpContext.Current;
            if (context != null)
            {
                context.Items[ScopeKey] = scope;
            }
        }

        return scope;
    }

    private Scope GetCurrentAsyncScope(Container container)
    {
        return HttpContext.Current != null 
            ? (Scope)HttpContext.Current.Items[ScopeKey]
            : base.GetCurrentScopeCore(container);
    }
}
dotnetjunkie commented 6 years ago

If I understand correctly, you wish to span parallel background operations from within a web request, and have them run in their own isolated scope.

The advised way of doing this is by using a hybrid lifestyle that falls back on the WebRequestLifestyle in case there is no active AsyncScopedLifestyle, while wrapping those background operations in an AsyncScopedLifestyle.

You can define your hybrid lifestyle as follows:

Lifestyle.CreateHybrid(
    defaultLifestyle: new AsyncScopedLifestyle(),
    fallbackLifestyle: new WebRequestLifestyle());

When spanning a background operation, you must make sure that that resolve of the required service and its execution is done within a Scope. Wrapping such operation can be done as follows:

using (AsyncScopedLifestyle.BeginScope(container))
{
    var service = container.GetInstance<ISomeService>();
    service.DoSomething(...);
}
malylemire1 commented 6 years ago

Hi, Actually it's not what I'm trying to do. There is no parallel execution. The problem is with Task.Run and parent ConfigureAwait(false). The AspNetSynchronizationContext is lost when the flow return from ConfigureAwait(false). So the HttpContext is lost. If you register something (like a DBContext) per WebResquest, trying to resolve it after the ConfigureAwait gives a lifetime scope exception.

The hybrid lifestyle will not work as it will create another DBContext after the ConfigureAwait(false) resume and will not have anything done within the WebRequestLifestyle.

WebRequestLifestyle should maintain scope event if AspNetSynchronizationContext is not propagated to a non parallel Task.Run in a web request.

That is what my version does.

Thanks

dotnetjunkie commented 6 years ago

In that case it would be better to simply go full-async. In other words, ditch the WebRequestLifestyle completely and just use AsyncScopedLifestyle and wrap a request in a Scope using AsyncScopedLifestyle.BeginScope.

malylemire1 commented 6 years ago

Hi,

I agree. I just wanted to avoid doing this in every method of every controllers.

Thanks

dotnetjunkie commented 6 years ago

I just wanted to avoid doing this in every method of every controllers.

Sure. That would be awful. You don't want that level of code repetition and controllers should be oblivious to this kind of infrastructure.

Instead, you should use some middleware that runs before and after each controller. In Web API you can achieve this with DelegatingHandlers. With MVC I think the way to achieve this is using a global filter.

malylemire1 commented 6 years ago

I'll try to implement this instead of using HttpContext.

Thanks