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

Turn "More than twenty 'IServiceProvider'..." warning to error by default #10236

Closed
ajcvickers opened this issue Nov 7, 2017 · 12 comments
Closed
Assignees
Labels
breaking-change closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-enhancement
Milestone

Comments

@ajcvickers
Copy link
Member

Since the application is almost certainly doing something wrong. Note this is a breaking changes and so probably can't happen until 3.0.

@ajcvickers
Copy link
Member Author

Marking for re-triage. Our tests are perhaps the one place where having more than twenty internal service providers is expected, since we test all the uncommon situations that require new service providers. We need to have a plan for our tests before we make a change like this.

@ajcvickers ajcvickers removed this from the 3.0.0 milestone Jan 6, 2019
@ajcvickers
Copy link
Member Author

Triage: Try not using the internal cache in our tests...

@ajcvickers ajcvickers added this to the 3.0.0 milestone Jan 7, 2019
ajcvickers added a commit that referenced this issue Feb 4, 2019
Issue #10236

Most of the changes here are two our tests where we use the internal service provider both as a convenience and to test specific behavior around the caching. The tests have been either:
* Updated to use an external service provider explicitly
* Updated to make use of a new option `ServiceProviderCachingEnabled`, which is on by default, but can be switched off to still use the internal service provider, but not to cache it.
* Left untouched where the behavior being tested depends on service provider caching.

This means our tests will not hit the error. We have a lot of testing of the general warning-as-error mechanism. Tested the new default throw here manually.
ajcvickers added a commit that referenced this issue Feb 4, 2019
Issue #10236

Most of the changes here are two our tests where we use the internal service provider both as a convenience and to test specific behavior around the caching. The tests have been either:
* Updated to use an external service provider explicitly
* Updated to make use of a new option `ServiceProviderCachingEnabled`, which is on by default, but can be switched off to still use the internal service provider, but not to cache it.
* Left untouched where the behavior being tested depends on service provider caching.

This means our tests will not hit the error. We have a lot of testing of the general warning-as-error mechanism. Tested the new default throw here manually.
ajcvickers added a commit that referenced this issue Feb 5, 2019
Issue #10236

Most of the changes here are two our tests where we use the internal service provider both as a convenience and to test specific behavior around the caching. The tests have been either:
* Updated to use an external service provider explicitly
* Updated to make use of a new option `ServiceProviderCachingEnabled`, which is on by default, but can be switched off to still use the internal service provider, but not to cache it.
* Left untouched where the behavior being tested depends on service provider caching.

This means our tests will not hit the error. We have a lot of testing of the general warning-as-error mechanism. Tested the new default throw here manually.
@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 Feb 6, 2019
@ajcvickers
Copy link
Member Author

Further food for thought on this one. The Identity tests failed and the root cause is interesting because it doesn't directly involve calling UseLoggerFactory or anything else "obviously wrong". Instead, if you setup ASP.NET tests to each run with its own isolated IServiceProvider, then a simple call to AddDbContext when setting up this service provider will result in a different ILoggerFactory and IMemoryCache being used for each test, which results in a new internal service provider per test. Hence boom.

@ajcvickers ajcvickers removed this from the 3.0.0 milestone Feb 8, 2019
@divega
Copy link
Contributor

divega commented Feb 8, 2019

@ajcvickers do we keep track of the application service provider? Would it be possible to throw only when too many internal service providers have been created for the same application service provider?

@ajcvickers
Copy link
Member Author

@divega Yeah, it would be. However, I'm not sure that's the right thing to do, because the case, as hit by the Identity tests, is pathological. Consider, each test creates and throws away an app service provider, but each time it does this it adds to the collection of internal service providers, which grows without bounds. So, it would be "really great"(TM) if we could find some way to prevent this. I've been trying to figure out if we can somehow know that the root of the app service provider has been disposed, and use that to flush the entity from our cache.

@divega
Copy link
Contributor

divega commented Feb 9, 2019

@ajcvickers I think I understand. I believe the detail that I was missing is precisely that the collection of internal service providers isn't really rooted on the application service provider.

@divega
Copy link
Contributor

divega commented Feb 9, 2019

I've been trying to figure out if we can somehow know that the root of the app service provider has been disposed, and use that to flush the entity from our cache.

@ajcvickers could we register a disposable service to keep track of the lifetime of the container?

@ajcvickers
Copy link
Member Author

@divega That's along the lines I was thinking. Problem is, the root IServiceProvider is usually not disposed.

@divega
Copy link
Contributor

divega commented Feb 10, 2019

Maybe that could be considered a bug in the tests 😄

@ajcvickers
Copy link
Member Author

@divega I thought about that too, but I don't think it really holds.

@divega
Copy link
Contributor

divega commented Feb 12, 2019

@ajcvickers FWIW, I believe during the triage this discussion I was confused about what you were proposing. Now I think you are saying make logger factory scoped to the DbContext and stop taking an external memory cache. Is this correct?

@ajcvickers
Copy link
Member Author

@divega Yes.

@ajcvickers ajcvickers added this to the 3.0.0 milestone Feb 15, 2019
@ajcvickers ajcvickers modified the milestones: 3.0.0, 3.0.0-preview3 Feb 22, 2019
roji added a commit to roji/efcore.pg that referenced this issue Mar 7, 2019
roji added a commit to roji/efcore.pg that referenced this issue Mar 7, 2019
roji added a commit to roji/efcore.pg that referenced this issue Mar 8, 2019
@ajcvickers ajcvickers modified the milestones: 3.0.0-preview3, 3.0.0 Nov 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-enhancement
Projects
None yet
Development

No branches or pull requests

3 participants