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

Move YesSql Initialize outside the service configuration #13147

Merged
merged 4 commits into from Jan 26, 2023

Conversation

MikeAlhayek
Copy link
Member

@MikeAlhayek MikeAlhayek commented Jan 26, 2023

Fix #13061

@jtkech you may want to check out the changes in ShellScope as it may not be a good idea.

I'll work on this tomorrow. But feel free to mess around with the PR if you like.

@jtkech
Copy link
Member

jtkech commented Jan 26, 2023

Yes maybe something like this but the goal of _serviceScopeOnly is to build a light scope e.g. without any tenant event, for example ActivatingAsync() triggering data migrations.

But Store is a tenant singleton so I'm more thinking about a tenant level registration. At the scope level we already RegisterBeforeDispose() a delegate to commit the session, so maybe something like this at the tenant level to ensure the Store initialization. E.g. in CreateDescribedContextAsync() we already call settings.EnsureConfigurationAsync() to init lazily shell config in an async way.

Will see tomorrow ;)

@MikeAlhayek MikeAlhayek added this to the 1.6 milestone Jan 26, 2023
@jtkech
Copy link
Member

jtkech commented Jan 26, 2023

Yes, this is the right fix as I did locally but ATM with a singleton resolving IStore and IOptions<StoreCollectionOptions> itself.

Also I had an use case where I needed to check if the resolved IStore is null. In my case I inject it so it's like a .GetRequiredService() but even if the resolution doesn't fail, we have cases where we explicitly return null.

Wait before merging so that I can try it and do a last review.

@sebastienros
Copy link
Member

@jtkech since I approved, please merge it when you are ok with it.

@jtkech
Copy link
Member

jtkech commented Jan 26, 2023

Okay

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.

Thread pool Starvation caused by sync over async pattern
3 participants