ReuseScope.Request

[jezzsantos]

Hey @mythz,

You gave me some good guidance in a previous post: Moving Services to Plugins

Which I took seriously, since you’ve been saying this a few times over the years. So I enacted that guidance and normalised all dependency registrations to either AddSingleton() or AddTransient() which turned out to be a big job across all our services and libraries.

However, its not quite the complete guidance, as I discovered the hard way.
You were right that in general we should be using AddSingleton() and AddTransient() based on whether the dependency (and its sub-dependencies) are thread-safe or not.
However, there are still those dependencies which are dependent on the current IRequest, and making those AddTransient() is where things have gone dramatically wrong at runtime.

For example, we have some dependencies that provide information based on the current IRequest. Our services depend on these dependencies having different values for each request, so they cannot be singleton obviously. However, they cannot be transient either, since they need the IRequest and something has to inject the IRequest into them as a dependency, in order for them to report on the data that is dependent on the IRequest.

What we currently do is have a GlobalRequestFilter that runs and look for dependencies that implement IRequiresRequest and for those dependencies we inject the IRequest into that dependency so it can work properly.
In these dependencies if they are transient, and they are used more than once within the same IRequest then they lose their IRequest after the first use, and subsequent uses have no IRequest anymore.

We either need to make these ones (a) AddScoped() or (b) find another way to get them access to the current IRequest and make them AddTransient() or AddSingleton() even.

Option (a) is possible with the current mechanism,
Option (b) requires some other way tot get the IRequest when the dependency needs it.

We discovered some time back that we could have class like this:

    public class HttpRequestFactory : IHttpRequestFactory
    {
        public IRequest Get()
        {
            var httpRequest = HostContext.GetCurrentRequest();
            return httpRequest ?? new BasicRequest();
        }
    }

we can now add this to the container as AddSingleton() since its a factory.
and have IHttpRequestFactory as a property on other dependencies that need the IRequest, and those dependencies can even be singletons.

public class SubDomainDetective
{
    public IHttpRequestFactory HttpRequestFactory { get; set; }

    public string GetCurrentDomain()
    {
        var request = HttpRequestFactory.Get();
        return request.Host;
    }
}

Which option above would you recommend? is there an option ©?

[mythz]

The IRequest is like Request DTO, i.e. per request data that shouldn’t be registered in the IOC, instead pass it in the method for anything that needs it as you would do Request DTO properties, e.g:

public MyDep MyDep { get; set; }

public object Any(MyRequest request) => MyDep.Method(base.Request, request.Id);

If you need to share any object throughout the Request pipeline, attach it to IRequest.Items[] and pass the IRequest in as an argument with your method. We use this for anything that requires access to per-request resources, e.g. Encryption Keys used in EncryptedMessages Feature, Stopwatch in Request Logging, Cache Metadata instructions in cache features, etc.

[jezzsantos]

OK, thanks,

I understand where you are coming from, but from your example here I dont see a very composable pattern for giving the request to some set or arbitrary dependencies that may need the IRequest, that may or may not be used by the current operation. I dont want to bake in that knowledge to each and every operation especially for cross cutting concerns of the whole service, like multitenancy.

I have to believe there is a better way?

Perhaps with a GlobalRequestFilter?

[mythz]

This is the cleanest and most testable approach with predictable behavior across all AppHosts, IOCs, etc as it’s not reliant on external environment thread request context switching implementations. A more testable approach would be to pass in the Request properties you need instead of IRequest but as that requires more code/effort, I prefer to just pass in IRequest when needed.

Perhaps with a GlobalRequestFilter?

The GlobalRequestFilter (or any other Request filter) is what you’d use to inject any per-request dependencies in IRequest.Items at the start of the Request pipeline.

Injecting IRequest works in both transient and singleton dependencies given it’s how to make dependencies ThreadSafe by passing in local dependencies as arguments.

Lazy Properties

Other than you could have a lazy property in a Service base class which constructs the dependency with the IRequest context which is how all multi-tenancy aware dependencies in Service.cs are implemented, e.g:

private IDbConnection db;
public virtual IDbConnection Db => db ?? (db = AppHost.GetDbConnection(Request));

Where your Services would be able to access it like a normal dependency:

public object Any(MyRequest request) => Db.SingleById<Table>(request.Id);

If you adopt this approach you’ll need to manually dispose of your dependency if it needs disposing. If you didn’t want a base class you could wrap the construction logic behind extension methods instead.

[jezzsantos]

Thanks @mythz, thanks for your patience.

I have to be honest with you, I am really struggling with taking this approach of passing through the IRequest from the service to all dependencies down the call chain.

In the way that we are doing multi-tenancy, we need to extract the Hostname to tell us which tenant the request is for, and that piece of data is then used to load database connection stuff from appsettings that are specific to that tenant (and unique for all tenants). Which means that we have a bunch of downstream dependencies including: a ICacheClientFactory and even AppSettings dependencies that need to return different information depending on which tenant the request is for. All of these dependencies need to know which tenant the current request right at the top of the request pipeline.

These dependencies are needed by a bunch of other higher level dependencies that are used by various services.
The idea is that we can just inject the higher level dependencies that each service needs without needing the service to care whether they are multi-tenanted aware or not. Multi-tenancy, like Authorization being a cross-cutting concern.

What that means is that, in order to pass the IRequest down the call chain, to these low level dependencies, we have to pollute all the other dependencies with the IRequest, which adds a bunch of complexity.

What I would like to have is a ICurrentTenantFactory dependency that somehow gets access the IRequest at the time it is used, so it can just return the current tenant, and have all other dependencies have a ICurrentTenantFactory, so that the dependencies that need to know whothe current tenant is, don’t need to know anything about the need for the IRequest.

So my question is: Is there another pattern that should be used for the one dependency (ICurrentTenantFactory to get access to the IRequest), so that any other dependency can get access to the current tenant by being injected with an instance of the ICurrentTenantFactory?

[mythz]

There isn’t, you’d need to access the Request Context via a singleton, i.e. HostContext.TryGetCurrentRequest() but that’s only available in ASP.NET by default and has a perf impact in .NET Core, in addition to all the other coupling and testability issues of depending on singletons.

If you need to do this for multiple dependencies, you could design your classes to pass your own “runtime context” (which includes IRequest) down to your dependencies that way you’ll only ever need to pass a single argument down in each method.

Don’t really have anything else to add other than for new functionality I’d design it with consideration that runtime arguments are passed as arguments instead of trying to resolve them as dependencies.

[jezzsantos]

For example, what is the problem with doing the following?

public override void Configure(Container container)
{
    container.AddScoped<ICurrentTenantFactory, HttpRequestCurrentTenantFactory>();

    this.GlobalRequestFilters.Add((req, res, dto)=> {
       var factory = req.TryResolve<ICurrentTenantFactory>();
       factory.Request = req; //Prime the factory with the current request
    });
}

and then later in the service operation:

internal class MyService : ServiceBase
{
    public ICurrentTenantFactory CurrentTenant { get; set; }

    public MyDtoResponse Get(MyDto request)
    {
        var tenant = CurrentTenant.Get();
        // Do something useful for the current tenant
    }
}

[mythz]

You’re relying on Request Scope. I’d invert the relationship and attach the tenant configuration to the Request:

GlobalRequestFilters.Add((req, res, dto)=> {
    req.Items[nameof(ICurrentTenantFactory)] = new HttpRequestCurrentTenantFactory(req);
});

Then in your Service have an extension method that pulls it from the IRequest.Items:

public object Get(MyDto request)
{
    var tenant = base.Request.GetTennant();
}

Although I don’t see why you’d need a factory and can just pass the tenant instance instead.

[jezzsantos]

That is still no good to me, because I still need access to the IRequest to get access to the current tenant, wherever I need the current tenant.

My example above was simplified of the real problem, which is other dependencies needing the current tenant to do their stuff.

[jezzsantos]

Let me expand the example:

public override void Configure(Container container)
{
    container.AddScoped<ICurrentTenantFactory, HttpRequestCurrentTenantFactory>();
    container.AddTransient<IAppSettings, TenantedAppSettings>();
    container.AddTransient<IConfigStore , TenantedStore>();

    this.GlobalRequestFilters.Add((req, res, dto)=> {
           var factory = req.TryResolve<ICurrentTenantFactory>();
           factory.Request = req; //Prime the factory with the current request
        });
}

public class TenantedAppSettings : IAppSettings
{
     public ICurrentTenantFactory CurrentTenant { get; set; }

     public IConfigStore Storage { get; set; }

         public string Get(string settingName)
         {
             var tenant = CurrentTenant.Get();
             return Storage.Get(tenant, settingName);
         }
}

now the service:

internal class MyService : ServiceBase
{
    public IAppSettings AppSettings { get; set; }

    public MyDtoResponse Get(MyDto request)
        {
        // Get the configuration for this tenant
        var setting = AppSettings.Get("somekey");
        }
}

[mythz]

My example above was simplified of the real problem, which is other dependencies needing the current tenant to do their stuff.

That’s what I’ve been referring to all this time, pass the request context down as an argument. If you don’t then you need to rely on the external hosting environment to maintain a runtime Request context across async/thread context switches which is implemented differently per hosting environment.

If you think about how can a singleton dependency access the current Request context that’s not passed as an argument, it needs to pluck it from a singleton which needs to be maintained across Thread hops which is why it has detrimental performance impact and disabled by default in .NET Core.

[jezzsantos]

I understand that, and want to avoid it too.
But surely the GlobalRequestFilter + Scoped dependency gives me a solution that achieves the same outcome?

[mythz]

A scoped dependency means relying on Request Scope which relies on the host-specific RequestContext.Instance behind the scenes and can’t be injected in singletons.

RequestContext.Instance needs to use AsyncLocal<T> in .NET Core (which has a perf impact), HttpContext.Current in ASP.NET which is only available within the context of a Request and is subjectable to runtime issues by trying to access it on background threads, WCF’s Remoting CallContext in HttpListener/MQ Hosts/.NET Framework Unit tests, which didn’t work properly in Mono for years and had to use [ThreadStatic] which doesn’t support async requests.

This thread is about avoiding Request Scope, the only way to do that is by passing per-request data as an argument otherwise you need to rely on the hosting environment to maintain logical thread storage which comes at a cost.

[jezzsantos]

I see. So we are saying that AddScoped() is off the table, and so all that is left now is:

Passing the IRequest down the call chain all the way to all dependencies that need it, or that need dependencies that need it. that sucks for me.

[mythz]

I mean you can use it, but I never do or need to.

[jezzsantos]

Thanks, I realise that going against your better judgement may not be wise, given you control the destiny of the product (and future support for these features, which I hope is not in jeopardy), but at this time I have to choose to reduce complexity and coupling over perf at this time.

Perhaps at some date in the future, I may have to tackle the inevitable.

thanks for your patience and the discourse