Skip to content

Update SetupAllEntities.cs #67

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

Merged
merged 1 commit into from Dec 9, 2022
Merged

Update SetupAllEntities.cs #67

merged 1 commit into from Dec 9, 2022

Conversation

ghost
Copy link

@ghost ghost commented Nov 24, 2022

While debugging my application, I discovered that one of my singleton services were being instantiated twice. I realized that this was due to GenericServicesSimpleSetup building the DI container (SetupAllEntities.cs:20) in order to resolve the DbContext. Since my DbContext depended on a singleton service, it was instantiated twice: once during setup, and again when the application built the root DI container.

My question is this: The temporary IServiceProvider that is built in SetupAllEntities doesn't seem to be used anywhere else. Could it be disposed after resolving the DbContext(s)? Currently you dispose the service scope, but this doesn't trigger Dispose on singleton services.

While debugging my application, I discovered that one of my singleton services were being instantiated twice. I realized that this was due to GenericServicesSimpleSetup building the DI container (SetupAllEntities.cs:20) in order to resolve the DbContext. Since my DbContext depended on a singleton service, it was instantiated twice: once during setup, and again when the application built the root DI container.

My question is this: The temporary IServiceProvider that is built in SetupAllEntities doesn't seem to be used anywhere else. Could it be disposed after resolving the DbContext(s)? Currently you dispose the service scope, but this doesn't trigger Dispose on singleton services.
@JonPSmith
Copy link
Owner

Hi @JonathanQuinth,

OK, I will look at that but it won't be immediately as I am in the middle of complex library.

I could accept your PR but I want to see if I can remove the building of the IServiceCollection as I now know that isn't a good thing to do.

@JonPSmith
Copy link
Owner

Hi @JonathanQuinth,

Version 5.2.0-preview001 of the GenericServices to not register the EF Core entities on startup, but when a entity is first used. It passes all the ~300 unit tests but could you try this NuGet in your app any let me know if it works. It definitely doesn't builds a temporary services on registration, but just check it works as normal.

Once you confirm it works, then I will release this version as 5.2.1.

@ghost
Copy link
Author

ghost commented Dec 9, 2022

Hej @JonPSmith,
Thanks for working on this fix. Unfortunately, I tried the NuGet package and immediately got a number of exceptions during setup. Please see the attached image.
Exceptions

I can analyze the problem in the context of my app a little more next week if that helps you.

Have a good one!

@JonPSmith JonPSmith merged commit 6348e3f into JonPSmith:master Dec 9, 2022
@JonPSmith
Copy link
Owner

I'm glad sent you a preview, but bad it didn't work! I did manage to get the unit tests to show the problem, but it also showed that it would be take a big change to the setup code, because so many things, like DTO matchings, AutoMapper setup, etc. are done startup.

I know that you aren't supposed to build the DI service while all the services haven't be set up but a redesign of the startup code is really difficult, so I have used this PR.

I have created a 5.2.1-preview001 for you check. I know you have done this yourself, but I would like you confirm the NuGet works before I release it.

@ghost
Copy link
Author

ghost commented Dec 12, 2022

Hi Jon!
My application works fine with this change applied.

@JonPSmith
Copy link
Owner

I have just released version 5.2.1 with the fix and unlisted the 5.2.1-preview001 version. Please swap over to version 5.2.1 when you can.

@ghost ghost deleted the patch-1 branch January 5, 2023 08:37
@ghost
Copy link
Author

ghost commented Jan 5, 2023

Thank you!

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.

1 participant