Skip to content

Conversation

@roji
Copy link
Member

@roji roji commented Apr 28, 2021

Alternative approach to #24768.

This hacky thing modifies AddDbContextFactory to also arrange for direct DbContext injection, via a lease mechanism that's very similar to what we already do for pooling. The user simply specifies the factory they want to use when setting up EF in DI, and do any customizations there.

For multi-tenancy, users would have to implement a factory like the following:

public class MultiTenantContextFactory : DbContextFactory<BlogContext>
{
    public MultiTenantContextFactory(
        IServiceProvider serviceProvider,
        DbContextOptions<BlogContext> options,
        IDbContextFactorySource<BlogContext> factorySource)
        : base(serviceProvider, options, factorySource)
    {
    }

    public override BlogContext CreateDbContext()
    {
        var context = base.CreateDbContext();
        // Fetch tenant id and inject it into the context
        return context;
    }
}

... and register it in DI as usual with AddDbContextFactory.

The big limitation at the moment is that IDbContextFactory manages creation, but not disposal. A bigger refactor could add Dispose or Return methods there, at which point it seems like DbContextPool could also be merged into PooledDbContextFactory (calling DbContext.Dispose would call into its factory's Return).

If we go down this route, users could also extend PooledDbContextFactory, and override the factory's methods to do nothing, as a way of getting rid of config snapshotting/resetting for ultra-high-perf (#24769), without needing a new flag and code path.

(The only thing I personally dislike is the additional ScopedDbContextLease which we need to disposal... if only the DI scope provider allowed us to register the DbContext for disposal instead...)

Complete sample user code
var serviceCollection = new ServiceCollection();
serviceCollection.AddDbContextFactory<BlogContext, MultiTenantContextFactory>(o =>
    o.UseSqlServer(@"Server=localhost;Database=test;User=SA;Password=Abcd5678;Connect Timeout=60;ConnectRetryCount=0"));
using var serviceProvider = serviceCollection.BuildServiceProvider();
using var scope = serviceProvider.CreateScope();
var scopedServiceProvider = scope.ServiceProvider;

using var ctx = scopedServiceProvider.GetService<BlogContext>();

public class MultiTenantContextFactory : DbContextFactory<BlogContext>
{
    public MultiTenantContextFactory(
        IServiceProvider serviceProvider,
        DbContextOptions<BlogContext> options,
        IDbContextFactorySource<BlogContext> factorySource)
        : base(serviceProvider, options, factorySource)
    {
    }

    public override BlogContext CreateDbContext()
    {
        var context = base.CreateDbContext();
        // Fetch tenant id and inject it into the context
        return context;
    }
}

public class BlogContext : DbContext
{
    public BlogContext(DbContextOptions<BlogContext> options) : base(options) {}

    public DbSet<Blog> Blogs { get; set; }
}

public class Blog
{
    public int Id { get; set; }
    public string Name { get; set; }
}

/cc @ajcvickers

@smitpatel
Copy link
Contributor

We should also find out that highly-voted issue where people requested the solution to their problem and post it there to gather feedback. This PR is very likely to be ignored by actual customers.

@roji
Copy link
Member Author

roji commented Apr 28, 2021

@smitpatel for sure, this is just an alternative design proposal. @ajcvickers do you know which issue it was?

@roji
Copy link
Member Author

roji commented Apr 29, 2021

Am guessing maybe this is the issue: #626

@AndriySvyryd
Copy link
Member

@roji I don't fully understand what this change is trying to accomplish.

AddDbContextFactory is designed to be called together with AddDbContext if you need to resolve both and neither is affected by #24768, since that's about pooling.

If you want both to use the same IDbContextFactory then AddDbContext should be modified instead. However you need to be careful with registering scoped services that use GetRequiredService in the factory lambda as that adds significant perf overhead.

@roji
Copy link
Member Author

roji commented May 3, 2021

The point here is for the user to register their own factory implementation, but to have contexts from that implementation get injected directly via DI - and not the factory itself. Right now, if you register a factory, your controllers only get injected with that factory. This would allow people to use their own factories - include lifecycle stuff (like tenant ID) - but still get injected with a context (and not with a factory). Does that make sense?

(this isn't an actual API proposal - it may be better to allow the user to specify their factory in AddDbContext instead).

If you want both to use the same IDbContextFactory then AddDbContext should be modified instead. However you need to be careful with registering scoped services that use GetRequiredService in the factory lambda as that adds significant perf overhead.

This is already how the existing AddDbContextPool is implemented. This is indeed not optimal; for the ultra-high-perf mode, we'd probably recommend getting injected with the factory directly (not with the context) to avoid this lookup (am doing something like this for the TE Fortunes platform benchmark).

(another option is to have a static factory, and reference that directly from the scoped lambda, but that's going outside of DI...).

@roji
Copy link
Member Author

roji commented Aug 17, 2021

Closing this for now, we'll revisit DI, factories and pooling in 7.0.

@roji roji closed this Aug 17, 2021
@smitpatel
Copy link
Contributor

@roji - Can you save the commit if you want to in your fork so we can delete branch from here?

@roji roji deleted the FactoryGames branch October 4, 2021 18:50
@roji
Copy link
Member Author

roji commented Oct 4, 2021

All good, deleted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants