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

Expose public API to reset a state of DbContext instance #15577

Closed
dasMulli opened this issue May 2, 2019 · 11 comments · Fixed by #21169
Closed

Expose public API to reset a state of DbContext instance #15577

dasMulli opened this issue May 2, 2019 · 11 comments · Fixed by #21169
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-enhancement
Milestone

Comments

@dasMulli
Copy link

dasMulli commented May 2, 2019

pooling db context instances is a great way to improve performance but it is only accessible when using DI contexts.

When migrating existing applications from EF classic to EF Core, it may be beneficial to use DbContextPool directly, since the application may not use DI or use different approaches (esp. with regard to scoping) to DI not covered by the EF-provided extension method for dependency injection configuration.

By making DbContextPool<TContext> a public API (instead of pubinternal), this would allow existing (large) applications to use pooling without the fear of APIs changing (a lot), even though it means that using it correctly could be an issue - but this is the same for correctly using types like ArrayPool<T>.

@ajcvickers
Copy link
Member

@dasMulli Have you tested that context pooling improves your perf? (It can, but only in limited scenarios. Most of the time the difference is barely measurable.)

@dasMulli
Copy link
Author

dasMulli commented May 2, 2019

I'll get some more measurements next week.
What were those limited scenarios? Just to make sure I'm measuring the right thing and not something different by accident..

@ajcvickers
Copy link
Member

@dasMulli Typically when doing nothing else with EF other than returning a single entity instance from a single, simple no-tracking query, at very high throughput.

@dasMulli
Copy link
Author

dasMulli commented May 2, 2019

Well.. looks like the app in question is doing that. Will look into more detailed measurement.

@jzabroski
Copy link

@dasMulli In my own research, there appears to be several different knobs to turn to figure out optimal throughput. See for example this wonderful StackOverflow answer about using the Async vs. Synchronous APIs for SqlClient: https://stackoverflow.com/a/42419159/1040437

I have been slowly cataloging over the last 2+ years a lot of such war stories, so let me know how you make out. I've personally started using benchmarkDotNet and it's a wonderful tool for simulating some assumptions. While it's artificial in some regards (you're not creating sockets over a network of clients), its reproducible and hands down that wins over minor artificiality.

@ajcvickers ajcvickers changed the title Make DBContextPool<TContext> a public API Expose public API to reset a state of DbContext instance May 10, 2019
@ajcvickers
Copy link
Member

Notes from triage: putting this on the backlog to consider exposing this. Specifically:

  • While short-lived context instances are recommended and a reset flag can lead people down this path, it is still valid in some cases to allow the context instance to be reset--for example, the scenario described in this issue.
  • Context pooling has limitations as to when resetting is safe to do. We need to make sure that if we expose resetting publicly, then we don't make it a pit of failure by allowing it to happen in situations where it is not safe.
  • We should consider the implications for resetting application state in the context instance.

@ajcvickers
Copy link
Member

ajcvickers commented Oct 22, 2019

Experience from the 3.0 release (see #17784 #18406) indicates that people are doing mass-detach of all entities because they need to clean the context and can't create a new instance. This adds some evidence to the need for this since it seems we're fighting a losing battle telling people not to keep their context instances around.

@jzabroski
Copy link

jzabroski commented Oct 22, 2019

@ajcvickers I think the main reason you are losing the battle is their is no clear DI pattern for most end-users. This is why DbContextScope GitHub project gained popularity in EF6 .NET Framework paradigm.

What I've done in the past is facilitated DI of Func<T> where T is IUnitOfWork / IRepository (or DbContext if you don't want to abstract out your DbContext from your business logic). My DI Container understands Func<T> is internally given a proxy that holds hte DI Container and calls Resolve on that generic <T>. Then, you just need an ITransactionManager with an OpenScope method that allows people to call the Func over and over again to get the Current Instance for the Current Scope. I've put this pattern in place with teams of 30 developers and they catch on quick

You lose when two things happen:

  1. Pop culture - things become popular faster than education supply can facilitate demand.
  2. You don't educate.

@kemmis
Copy link

kemmis commented Jan 2, 2020

@jzabroski Can you elaborate on how you're accomplishing the DbContextScope paradigm using EF Core 3 now? My team has been using DbContextScope up through EF Core 2.2, but with EF Core 3, some of the essential parts have gone away within EF. So we don't have a direct upgrade path to EF Core 3 from 2.2.

With 2.2, we registered IAmbientDbContextLocator and IDbContextScopeFactory with the .NET Core DI system, and all our classes get those injected like magic. Is there a standard way in EF Core that should be used to just let the DI in .NET Core manage the scopes and handle SaveChanges() at the scope level at the outter-most SaveChanges() call in a scope, just like DbContextScope does it or you?

SOS. Send help immediately. 🆘

@jzabroski
Copy link

jzabroski commented Jan 2, 2020

I don't actually use DbContextScope - I use dependency injection to ensure scopes. Autofac has the concept of Delegate Factories, and other contains have similar concepts. (Tangent: I actually don't like DbContextScope, as it combines too many concerns, and I like narrow interfaces when creating a facade around a much bigger system.)

In general, I agree with @ajcvickers decision to tell people not to keep their contexts around. But then the question becomes, what's the cheapest way to get a new context? You need a new DI Scope.

See:
mehdime/DbContextScope#29 (comment)
and
https://autofaccn.readthedocs.io/en/latest/advanced/delegate-factories.html

Note, I deliberately use public class Example because the point is this is a general trick to hide state that should be in any OO programmers "bag of tricks". By hiding the DbContext behind any class, you can actually share the context across all classes consuming that context. This enhances composition since you don't need to enable evil things like MSDTC in order to get all Save commands to belong to a single ACID Transaction. And by making things a Func<T> F, all you need to do is open a new DI transaction scope and call F() to get a new object inside a new scope.

On some projects, I have Save inside my IRepository. In others, I split it out so that Save is actually interface IUnitOfWork { void Add(object o); void Delete(object o); void Commit(); } In others, I split out UnitOfWork further so that I have an IUnitOfWorkParticipant { void Enlist(); }. In such scenarios, I don't need to care about how many data access layer changes from release to release, I just need to plug-in the context as if it was a single thread of execution. At this point, all I'm doing is grouping objects together and using DI to create a logically single thread of computation. There's only one call stack. This obviously means this approach is embarassingly parallel, because all you need to do is use your DI Container to create parallel DbContext(s). As long as the ORM provides a thread-safe DbContext and your database supports multi-threaded execution (cough, don't use SQLite, cough), you can create as many of these as you want.

As for why I don't always abstract things into the most narrow interfaces with the most narrow concerns? I don't always believe in SOLID, especially Single Responsibility Principle. All I care about is that I can write tests that define properties of the interface for all classes that implement that interface.

@AndriySvyryd
Copy link
Member

Related: #18575

@ajcvickers ajcvickers modified the milestones: Backlog, 5.0.0 Jun 1, 2020
@ajcvickers ajcvickers self-assigned this Jun 5, 2020
ajcvickers added a commit that referenced this issue Jun 7, 2020
Fixes #15577

This is an alternative to mass-detach for situations where creating a new context instance is difficult.

The context configuration is not reset, since it seems likely that setting like lazy-loading and registered events are more useful left as they would be--just as is the case when doing mass-detach.
@ajcvickers ajcvickers added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Jun 7, 2020
ajcvickers added a commit that referenced this issue Jun 8, 2020
Fixes #15577

This is an alternative to mass-detach for situations where creating a new context instance is difficult.

The context configuration is not reset, since it seems likely that setting like lazy-loading and registered events are more useful left as they would be--just as is the case when doing mass-detach.
ajcvickers added a commit that referenced this issue Jun 8, 2020
Fixes #15577

This is an alternative to mass-detach for situations where creating a new context instance is difficult.

The context configuration is not reset, since it seems likely that setting like lazy-loading and registered events are more useful left as they would be--just as is the case when doing mass-detach.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants