Skip to content
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

Make sure that scoping with AddDbContext is a good experience #1112

Closed
ajcvickers opened this issue Nov 21, 2014 · 6 comments
Closed

Make sure that scoping with AddDbContext is a good experience #1112

ajcvickers opened this issue Nov 21, 2014 · 6 comments

Comments

@ajcvickers
Copy link
Member

AddDbContext registers the application's context as a scoped service. This means that within a given scope the same instance is always returned. This is fine when something (e.g. a web framework) is creating and disposing the scope and the application does not also attempt to dispose the context. However, it means that a case like this does not in a not very obvious way:

var serviceProvider
    = new ServiceCollection()
        .AddEntityFramework()
        .AddSqlServer()
        .AddDbContext<SchemaContext>()
        .ServiceCollection
        .BuildServiceProvider();

using (var context = serviceProvider.GetRequiredService<SchemaContext>())
{
    // Do stuff
}

using (var context = serviceProvider.GetRequiredService<SchemaContext>())
{
  // Do stuff expecting a different, non-disposed context instance
}

In this code the second using actually gets the same context instance again, which has already been disposed. The solution right now would be to create a DI scope for each using and dispose that scope, which in turn will dispose each context. For example:

var serviceProvider
    = new ServiceCollection()
        .AddEntityFramework()
        .AddSqlServer()
        .AddDbContext<SchemaContext>()
        .ServiceCollection
        .BuildServiceProvider();

using (var scope = serviceProvider.GetRequiredService<IServiceScopeFactory>().CreateScope()
{
    var context = scope.ServiceProvider.GetRequiredService<SchemaContext>())
    // Do stuff
}

using (var scope = serviceProvider.GetRequiredService<IServiceScopeFactory>().CreateScope()
{
    var context = scope.ServiceProvider.GetRequiredService<SchemaContext>())
    // Do stuff in a new, non-disposed context instance
}

But this is cumbersome and non-obvious. So can we come up with patterns/guidelines/pits of success such that:

  • Making sure a context gets disposed is easy
  • A new context instance is returned when appropriate with code that a developer is likely to write
@divega
Copy link
Contributor

divega commented Nov 21, 2014

Some quick (crazy?) ideas that come to mind, that maybe we can discuss later:

  1. Rename AddDbContext to something like AddScopedDbContext or AddPerRequestDbContext and consider adding other versions of the method with different lifetimes if we think it is worth doing.
  2. Make it so that reusing an already disposed DbContext can resurrect it with fresh state.
  3. Facilitate/document a better pattern that people can use in code when they want control the lifetime of the context, e.g. with a using block. The pattern could be that they new up the DbContext but they pass a DbContextOptions from DI, or that they can inject the IServiceProvider, e.g. by using the DbContextActivator directly.

@rowanmiller rowanmiller added this to the 1.0.0 milestone Nov 24, 2014
@rowanmiller
Copy link
Contributor

I don't think we need to do anything too complex to try and make stuff work here. If you're managing the service provider like this then it's a pretty complex scenario and I think it's fine for you to know you need to create scopes etc.

@divega
Copy link
Contributor

divega commented Nov 25, 2014

I am not sure having some DI setup code in your application's start up means that you want to learn how to create your own DI scopes to control the lifetime of your DbContexts. That feels like a cliff. But I agree we shouldn't do anything too complex.

@ajcvickers
Copy link
Member Author

Decisions on this:

  • We will keep the DbContext constructors that take IServiceProvider. This is probably how most people using DI outside of EF will create their contexts. It's what we do in testing.
  • Given this, we will not make any changes to AddDbContext at this time--it will always register a scoped context and anyone using it should use it with an appropriate DI scope. This is an advanced scenario outside of ASP.

Clearing for re-triage since I don't think there is anything to actually do here.

@ajcvickers ajcvickers removed this from the 7.0.0 milestone Jan 9, 2015
@ajcvickers ajcvickers removed their assignment Jan 9, 2015
@divega
Copy link
Contributor

divega commented Jan 30, 2016

While responding to aspnet/DependencyInjection#352. I had what I think is a new idea that could help improve this experience. Here is a summary

  1. Add to the framework a DbContextFactory<TContext> class, e.g. shape would look somewhat like this:

    public class DbContextFactory<TContext>
    {
      public DbContextFactory(IServiceProvider serviceProvider, DbContextOptions<TContext> options)
      {
        ...
      }
      public TContext Create()
      {
        ...
      }
    }
  2. The AddDbContext() method would register this in DI.

  3. Users that need a single context per request scope would still resolve the TContext directly in constructors and users that need to create multiple instances could resolve the factory and then create the individual instances manually using the Create() method.

A few details:

  • This does not replace the internal DbContextActivator mechanism but would be mostly sugar built on top of it (we still need a static place to stash the IServiceProvider).
  • There are possibly other variations on the pattern to consider, e.g. make the class non-generic and the Create<TContext>() method generic, change where we pass the DbContextOptions, add a params object[] argument to Create(), etc.
  • We can change the registration in AddDbContext() to always go through an instance of this class or just leave it as it is now.

@ajcvickers I don't remember if we ever discussed doing something like this. Do you?

@divega divega reopened this Jan 30, 2016
@divega
Copy link
Contributor

divega commented Jan 30, 2016

Hmm, I just realized that this is probably the wrong bug as it was originally about the default scoping of AddDbContext more than about having multiple instances per request. I think we had a different one covering the latter but I cannot find it at the moment. I will create a new issue.

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

No branches or pull requests

3 participants