Skip to content

[Advice] Tackling multi-tenancy with stock DI - open generics #2964

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Antaris opened this issue Mar 15, 2018 · 13 comments
Closed

[Advice] Tackling multi-tenancy with stock DI - open generics #2964

Antaris opened this issue Mar 15, 2018 · 13 comments
Milestone

Comments

@Antaris
Copy link

Antaris commented Mar 15, 2018

Hi team,

I'm tackling a design around supporting multi-tenancy in my application. Up until now, I've entirely relied on the stock DI libraries for all my IoC needs. To handle multi-tenancy, I wanted to enable scenarios around service lifetime that segregates tenant-scoped services away from singletons, scoped and transient services.

I know saaskit has gone a long way to provide a generic approach to multienancy, but, they've specifically built there DI story around StructureMap, which I am looking to avoid doing. In a future ideal world, I'd like my approach to multi-tenancy to support any DI container that .NET Core supports and is IServiceProvider compatible.

My current approach is to introduce a subclass of ServiceDescriptor: the TenantScopedServiceDescriptor. This descriptor supports a fixed lifetime scope of Singleton, so if the mutli-tenancy feature is disabled, any services registered with my AddTenantScoped(...) are singleton on the root provider.

The way I am planning to handle tenant-scoped services, was to create a new IServiceProvider on request, once a TenantId is resolved. This tenant-scoped provider is cached for the lifetime of the application, like ApplicationServices is. Once I have the tenant-scoped service provider, the next logical step is to replace RequestServices with my own scoped instance from my tenant-scoped parent provider. This is handled through middleware.

The way this works under the hood, is when creating the tenant-scoped provider, I enumerate through all the available service registrations, and clone the service collection - except where I encounter a Singleton descriptor (which is not my TenantScopedServiceDescriptor). For singleton services, I create a new ServiceDescriptor which essentially delegates the instance resolution back to my root provider. The net effect should mean that transient and scoped services still behave as intended, but tenant-scoped services are resolved from my tenant-specific provider, and singletons from ApplicationServices:

IServiceCollection tenantServices = new ServiceCollection();
foreach (var descriptor in services)
{
    if (descriptor.Lifetime != ServiceLifetime.Singleton)
    {
        // MA - Scoped and transient service descriptors can be added directly.
        tenantServices.Add(descriptor);
    }
    else
    {
        if (descriptor is TenantScopedServiceDescriptor tssd)
        {
            // MA - Tenant scoped services become singletons in tenant-specific service provider.
            tenantServices.Add(descriptor);
        }
        else if (descriptor.ServiceType != typeof(IServiceCollection))
        {
            // MA - True singletons can be forwarded to the application services for resolution
            tenantServices.Add(new ServiceDescriptor(
                descriptor.ServiceType,
                sp => applicationServices.GetService(descriptor.ServiceType),
                ServiceLifetime.Transient));
        }
    }
}

Where this design is falling down, is that I can't create a forwarding ServiceDesriptor for open generics, as it requires an explicit open generic implementation type:

Open generic service type 'Microsoft.Extensions.Options.IOptions`1[TOptions]' requires registering an open generic implementation type.

I was hoping that there was an existing overload for Add* which would support Func<IServiceProvider, Type, object> for instance resolution, providing the second argument Type as the concrete service type being requested. I can see this has been raised previously in #588, #474 and #498 but each was closed without any real resolution.

Even if that were possible, it might require some additional changes under the hood to allow open generics to be delegated to an instance factory method - but even that has pitfalls in terms of type assurance etc.

Does anyone have advice - I'm specifically looking to stick around the stock DI rather than pull in another DI container.

@davidfowl
Copy link
Member

/cc @sebastienros

@sebastienros
Copy link
Member

We took this approach in Orchard Core, have you looked at how it's done there? I could suggest you to use Orchard Core directly as it will provide more than just multi-tenancy, things that you will probably need for SaaS applications (modularity, theming, admin panel, user/role management, permissions, openid, ...).

PS: Orchard Core and Orchard Core CMS are two different sets of packages, you won't need the CMS (people are often confused with the distinction)

@Antaris
Copy link
Author

Antaris commented Mar 16, 2018

@sebastienros Just reviewed how it is done in OrchardCore - its nice to know I was on the right track. I can see a couple of issues with the approach which I'm sure you've already considered

  • Adding the open generics as singletons in the tenant service collection effectively means they are not true singletons, a singleton instance could exist for each tenant - I had this same thought, I can't see anyway round it :-/
  • You eagerly grab other singletons directly from the root service provider and add them as singleton instances in the tenant service provider - are there any semantics around IDisposable not being executed for singleton instances that are not resolved in the container (@davidfowl?) - the thing I was considering is although a tenant service provider should live for the life of the application - could there be a situation where disposal of the tenant service provider could leave a singleton instance sourced from the root service provider in a disposed state?

It's an interesting challenge to try and tackle - I'll take a further look at OrchardCore - although I like the challenge of building stuff for myself - as does anyone I guess :-)

I know those issues I've mentioned have previously been closed and a lot of the development around DI is governed by what features other containers support, but is it worth looking into DI changes to support:

  • Adding an overload for resolving services to support Func<IServiceProvider, Type, object> delegates where Type is the service type (e.g. the closed generic type)
  • Resolving open generic services using a delegate (dependent on the above change?)

cc @pranavkm

@sebastienros
Copy link
Member

sebastienros commented Mar 16, 2018

The lifetime of the singletons is interesting, the way we register them means that they are not disposed once a tenant is stopped or recycled. This actually raised some concerns in the DI that the team handled quickly. So if we pass a custom instance, the DI won't try to dispose it, it's our instance. There is still an issue that is open which is about being explicit during the registration to provide the behavior regarding disposal. So yes right now the way we do it is not ideal with open generics.

although I like the challenge of building stuff for myself

Nobody will blame you on this one, we love puzzles and programming ;)

@muratg
Copy link
Contributor

muratg commented Apr 3, 2018

@Antaris @sebastienros Any further action needed here (i.e. actual DI code change or investigation)?

@sebastienros
Copy link
Member

@Antaris
Copy link
Author

Antaris commented Apr 3, 2018

@muratg

I think one of the stumbling blocks that has forced this design is the inability to close a generic type using a delegate instead of an open type service definition, for instance:

services.AddScoped(typeof(IGeneric<>), sp => SomeMethodToCloseGeneric());

Obviously, one of the kicks here is the delegates only pass the IServiceProvider, instead of some further context around the type being requested, e.g., something like:

(sp, t) => SomeMethodToCloseGeneric(t);

Or perhaps a some sort of ServiceContext containing the IServiceProvider as well as the ServiceType reference:

class ServiceContext
{
   public IServiceProvider ServiceProvider { get; }
   public Type ServiceType { get; }
}

I think a combination of those items is not out of the realms of possibilities given the cross-section of containers that we're trying to satisfy?

@davidfowl
Copy link
Member

If you can implement that on all the containers we support today then there's a chance, otherwise, it's a no.

@Antaris
Copy link
Author

Antaris commented Apr 3, 2018

@davidfowl What's your official supported list of containers? I don't mind investigating?

@Antaris
Copy link
Author

Antaris commented Apr 3, 2018

@sebastienros
Copy link
Member

@Antaris we also have a PR to fix some edge cases. OrchardCMS/OrchardCore#1644

@davidfowl
Copy link
Member

@davidfowl What's your official supported list of containers? I don't mind investigating?

We have no real official list (as we don't provide support), but you can use this as a starting point https://github.com/aspnet/DependencyInjection#using-other-containers-with-microsoftextensionsdependencyinjection.

@davidfowl davidfowl added this to the Backlog milestone Oct 17, 2018
@davidfowl
Copy link
Member

We need to figure out if this built on top (and therefore works for all container adapters) or if it's built into the implementation. So far all of the solutions on top have caveats but it's where we currently are. Maybe a doc describing the state of things and the patterns we recommend with tradeoffs is the action item here.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants