Efcore: AddDbContext<TContext>() is only useful if DbContext will be used in web requests in an ASP.NET application

[divega]

The main issue IMO is that AddDbContext<TContext>() always adds the context as a scoped service. This is very useful for the 80% scenario in ASP.NET in which a single DbContext instance is resolved and used per request, but it can be problematic or require additional knowledge (e.g. about DI scopes) if anyone is trying to use it outside this scenario, e.g.:

• Resolve an instance of TContext in other parts of the application, e.g. startup
• In a non-web application that uses DI
• In test code

E.g. the following code will fail:

``` C#
var serviceProvider = new ServiceCollection()
.AddEntityFrameworkSqlServer()
.AddDbContext(
options => options.UseSqlServer(_connectionString))
.BuildServiceProvider();

using (var context = serviceProvider.GetService())
{
// do some work
}

using (var context = serviceProvider.GetService())
{
// this will return the same instance which has already been disposed
}
```

There a couple of things I believe we could consider doing:

1. Rename AddDbContext<TContext>() to AddScopedDbContext<TContext>()
   
   
   
   • In addition we could create methods for different lifetimes, e.g. AddTransientDbContext<TContext>(), AddSingletonDbContext<TContext>() and keep using the scoped version in the default ASP.NET templates.
   
   
2. Move this method to a different package that is ASP.NET specific (possibly alongside other things that are ASP.NET specific, such as things that deal with Startup).

[rowanmiller]

Option 2 :+1: :smile:

BTW the example of wrapping services from a ServiceProvider in a using statement is generally a bad idea anyway. The ServiceProvider should be responsible for disposing anything it creates at the appropriate time.

[divega]

the example of wrapping services from a ServiceProvider in a using statement is generally a bad idea anyway

Agreed. The example is a bit artificial because disposing is not needed or necessarily a good idea (especially since the instance is scoped). The main point is the user didn't know that nor wanted the instances to be the same.

[divega]

@ajcvickers will come up with a proposal.

[dance]

Could you please take a look at https://github.com/aspnet/EntityFramework/compare/dev...dance:dbcontext-lifetimes if it is right direction to go? There are no tests yet - if you approve, i'll add them.
Some thoughts:

• better not add AddSingletonDbContext as it doesn't feel right and may be confusing
• go for option 1 as scoped context scenario doesn't necessarily belong to asp.net IMO

[sthiakos]

• the scoped dbcontext really does hit the 80%+ of my asp.net scenarios - it's a great way to make the request atomic especially when invoking a transaction at the beginning of the request and then committing the transaction on success or rolling back on exception/failure of the completed request.
• I use a dbcontextfactory/dbcontextprovider for when I need to control lifetime for the short "burst" of on-demand connections for the remaining asp.net 20% (usually in high volume/performance api endpoints) and other situations where I _might_ use the db connection (e.g. a secondary database used for cached data)

given the CommandContext example above:

public class CommandContextProvider
{
    public CommandContext CreateContext() { return new CommandContext(); }
}

use it:

using (var context = serviceProvider.GetService<CommandContextProvider>().CreateContext())
{
    // do some work
}

I see IOC containers performing two major activities in this issue - resolving dependencies and managing lifetimes. Because of the need to manage DbContext lifetimes explicitly (i.e. IDisposable), I think it makes sense to sometimes assume that responsibility in my code.

Not sure how you might factor this into your solution. Perhaps a general DbContextProvider<TContext>class that is registered with .AddDbContextProvider<TContext>().

My 2 cents.

Steve

P.S. - I advocate moving to aspnet package and leaving it with only the scoped lifetime (remaining as .AddDbContext<TContext>())

[ajcvickers]

Proposal for triage:

• Do not move to a separate package. While AddDbContext is going to be primarily used by ASP.NET applications it isn't actually ASP.NET specific. It is useful for anyone wanting to use EF with an external service provider.
• Keep the method name simple and defaulting to scoped, but provide overloads that make use of the existing ServiceLifetime enum to change the lifetime.
  
  
  
  • Not quite as discoverable that the default is scoped, but still there in the method signatures
  
  
  • Easy to change the lifetime for those that need it
  
  

Examples:
Default scoped:

``` C#
services.AddDbContext(
options => options.UseSqlServer(Configuration["Data:DefaultConnection:ConnectionString"]));

Transient:

``` C#
services.AddDbContext<ApplicationDbContext>(
    ServiceLifetime.Transient,
    options =>  options.UseSqlServer(Configuration["Data:DefaultConnection:ConnectionString"]));

[divega]

@ajcvickers catching up with this proposal and the PR. The proposal looks good in general, I only have one doubt, related to something @dance mentioned in https://github.com/aspnet/EntityFramework/issues/4988#issuecomment-208577077:

better not add AddSingletonDbContext as it doesn't feel right and may be confusing

I tend to agree that singleton will almost never be the right choice, but I might be missing something. It also seems to me that in our DI APIs only the lowest level methods tend to expose the ServiceLifetime enum. I see also in the PR there is a little bit of contention on the position of the lifetime parameter.

Putting all this together the option of having multiple methods (probably AddScopedDbContext<TContext>() and AddTransientDbContext<TContext>()) still seems more compelling in my mind.

Anyway, not pushing back on your proposal, just wondering if any of these is new data or if you took it into account :smile:

[ajcvickers]

@divega I'm fine with that, but we will break everybody going from RC2 to RTM.

[divega]

@ajcvickers and I talked in person, and we would like @rowanmiller to weigh in. It seems we have a few options to chose from:

1. Do nothing: Assume that it is ok for contexts resolved from DI to be always scoped and have people that need transient doing the registration manually or using new MyDbContext(options) (the options could still come from DI, which makes it still relatively nice).
2. Take the PR as is (or with the order of the parameters changed :smile:), i.e. just add an overload that takes the service life time (this has some possible disadvantages as I mentioned in https://github.com/aspnet/EntityFramework/issues/4988#issuecomment-210209288).
3. Have AddScopedDbContext<TContext>() and AddTransientDbContext<TContext>(). We would need to choose between breaking between RC2 and RTM or trying to get it through for RC2 in which case someone would go through the pain of a cross repro change (e.g. templates, Identity tests, samples and possibly other things use AddDbContext<TContext>() today) on both release and dev branches, but it might be worth doing if we think is the right thing.
4. Keep AddDbContext<TContext>() as the scoped version and add AddTransientDbContext<TContext>(). Compared to (2) this keeps it less clear what the code in Startup.ConfigureServices() is doing.

[ajcvickers]

I'm fine with any of options 2, 3, or 4.

[dance]

I vote for option 3. Option 4 is less preferable but still acceptable. I agree with @divega that

Compared to (2) this keeps it less clear what the code in Startup.ConfigureServices() is doing.

[mikes-gh]

Don't really like 2 seems like a band aid and not consistent naming wise.
3 ideally (painful)
4 OK

[rowanmiller]

2 or 3 seem fine to me. Lets discuss in person today and make a final decision.

[ajcvickers]

Team vote for 2 with the parameter order switched.