Add Async method to ContentDefinitionManager to prevent possible thread starvation

[MikeAlhayek]

Fix #14667

• Fix all the warning
• Cleanup the duplicated logic in the methods. Point the non-async methods to async by using .GetAwaiter().GetResult()
• document the changes to the interfaces.

Here is a list of interfaces that were modified

• IStereotypeService
• IStereotypesProvider
• IRouteableContentTypeProvider
• IRouteableContentTypeCoordinator
• IContentDefinitionService
• IContentDefinitionManager
• IContentDefinitionService

@sebastienros lots of changes had to happen here to ensure that IContentDefinitionManager is no longer blocking. I have another PR going into YesSql that would allow us to reduce the locking even more.

@jtkech lots of changes here. I would appreciate a second set of eye to ensure I did not miss anything.

[MikeAlhayek]

Fixed the failing tests. Did random tests on the site and everything seems to be working as expected.

[jtkech]

This is something that I already suggested when we introduced DocumentManager but at a time it was not so easy to make things approved ;) So because sometimes there are pros and cons to make things async we did a compromise that doesn't fully use the DocumentManager pattern.

For example it still uses directly the IMemoryCache.

So, if we agree to make it async I think that it should follow the regular DocumentManager pattern (should be done carefully), this is something that I can do.

I will look at it when I will have time.

[MikeAlhayek]

@jtkech

if we agree to make it async I think that it should follow the regular DocumentManager pattern (should be done carefully), this is something that I can do.

This is something @sebastienros and I chatted about. I think he is good with using async to prevent blocking. Do you see a problem with converting this to async?

[jtkech]

Do you see a problem with converting this to async?

Not at all, it was just for info about the history, but then should change for example the direct usage of the memory cache and its invalidation, I already did it but reverted it to allow an old PR to be approved.

For now I'have too many things in late but I will work on it asap.

Note: Good to improve things but we still need to understand what really happens e.g. in #14616.

[MikeAlhayek]

Note: Good to improve things but we still need to understand what really happens e.g. in #14616.

Absolutely, this is a big problem for me. But, the more I dig, the more things I notice and end up with a PR to improve. For example, I am under the impression that is something cause a lock of the resources and the only thread is not release. So now I am after anything that is causing a lock. Similarly, the PR in YesSql sebastienros/yessql#508 was submitted to prevent locking. Currently, event session.Save(obj) causes a lock which is not good since this method is used everywhere.

[jtkech]

Currently, event session.Save(obj) causes a lock

Do you mean that you are also saving data while processing your report? If so maybe a dead lock between 2 transactions waiting for each other. Otherwise if even a regular session save may cause a lock, maybe something else more specific to find. Because I don't have the context to be able to repro, can you share some parts of your code in case there is something to find.

[MikeAlhayek]

@jtkech no. The report reads only and does not save data. If there was a deadlock, we'll likely to hit that deadlock even when running the same report with less data.

My save locks, I mean in general the call to Save causes a thread lock. Keep in mind that I am running the app on a single vCore which means the CPU is able to process a single thread at a time. So if we don't release the lock, the CPU won't be able to process anything else meanwhile.

[jtkech]

Keep in mind that I am running the app on a single vCore which means the CPU is able to process a single thread at a time.

Ah okay, this may explain some behaviors, for example we have some synchronous locks but, for most of them, not to access IO resources, hmm are you using Lucene?

One good thing to test would be to see what happens when disabling step by step some features.

[jtkech]

Didn't have time to look at it, I will try asap.

As I remember it has some specificities that we may have to take care.

Otherwise we could just extend/change the interface, and then the implementation but moving old methods to interface/extensions methods for backward compatibilities, as we did elsewhere.

Just retrieved this 2 PRs ;) #6921 #3930

[jtkech]

@sebastienros @MikeAlhayek Okay Just saw the related meeting discussion.

As said when we introduced DocumentManager we used it for ContentDefinitionManager, I already suggestted to make it all async but because of pros and cons we didn't do it.

But ContentDefinitionManager already uses DocumentManager through IContentDefinitionStore to manage Get/Load for update, syncing between instances, it also already manages cumulative updates where data are persisted once at the end of the shell scope and only if the session is well committed.

So would need to fully follow the DocumentManager async pattern and see how we manage part that are always synchronous, didn't have time yet but I will look at it asap.

Here the database one.

OrchardCore/src/OrchardCore/OrchardCore.ContentManagement/DatabaseContentDefinitionStore.cs

Lines 7 to 24 in 8c30a17

	public class DatabaseContentDefinitionStore : IContentDefinitionStore
	{
	private readonly IDocumentManager<ContentDefinitionRecord> _documentManager;
	public DatabaseContentDefinitionStore(IDocumentManager<ContentDefinitionRecord> documentManager)
	{
	_documentManager = documentManager;
	}
	/// <inheritdoc />
	public Task<ContentDefinitionRecord> LoadContentDefinitionAsync() => _documentManager.GetOrCreateMutableAsync();
	/// <inheritdoc />
	public Task<ContentDefinitionRecord> GetContentDefinitionAsync() => _documentManager.GetOrCreateImmutableAsync();
	/// <inheritdoc />
	public Task SaveContentDefinitionAsync(ContentDefinitionRecord contentDefinitionRecord) => _documentManager.UpdateAsync(contentDefinitionRecord);
	}

There is also FileContentDefinitionStore using an IDocumentManager<IFileDocumentStore, ...>.

OrchardCore/src/OrchardCore/OrchardCore.ContentManagement/FileContentDefinitionStore.cs

Lines 8 to 10 in 8c30a17

	public class FileContentDefinitionStore : IContentDefinitionStore
	{
	private readonly IDocumentManager<IFileDocumentStore, ContentDefinitionRecord> _documentManager;

[jtkech]

Okay, I take a look and seems to be consistent, as said (see my above comment) the definition manager already use IDocumentManager through the IContentDefinitionStore.

Could be a singleton if we make the 2 stores singletons which is possible as they only inject IDocumentManager that we made singletons (even if they always execute in a scope context).

And we use the memory cache to GET already built types and parts definitions, but we use scoped caches when we do multiple LOADs for update in the same scope.

We can't GET once on startup as ContentDefinitionRecord is a shared Document that needs to be kept in sync, but most of the time this is what happens while rendering the frist content item.

As described when we LOAD for updating we act on the same scoped cached built definitions and the ContentDefinitionRecord Document is already persisted only once at the end of the shell scope.

So what @MikeAlhayek has already done seems to be almost what we need, good job.

We would just need to reduce the number of async calls, I think we can because I saw redundant calls, I will work on it very soon.

[jtkech]

@MikeAlhayek I started to work on it, for info ConfigureAwait(false) is useless under aspnetcore.

[MikeAlhayek]

@jtkech Yes I was planning to remove them. But, I left them alone until you made the changes you mentioned first.

[MikeAlhayek]

@jtkech I did some cleanup and removed some redundant async calls. I hope we can merge this PR soon.

As described when we LOAD for updating we act on the same scoped cached built definitions and the ContentDefinitionRecord Document is already persisted only once at the end of the shell scope.

Where in the code this is done to ensure we only update the database one?

[jtkech]

@MikeAlhayek

I did some cleanup and removed some redundant async calls. I hope we can merge this PR soon.

Okay, let me work on it this night.

Where in the code this is done to ensure we only update the database one?

SaveContentDefinitionAsync() > _documentManager.UpdateAsync() > DocumentStore.UpdateAsync() > _session.Save() and AfterCommitSuccess<T>() that registers a possible delegate.

Then when DocumentStore.CommitAsync() > _session.SaveChangesAsync() and execute possible delegates, knowing that CommitAsync() is called at the end of the shell scope because of how ISession is resolved, see the registration of ISession that contains the following.

                ShellScope.Current
                    .RegisterBeforeDispose(scope =>
                    {
                        return scope.ServiceProvider
                            .GetRequiredService<IDocumentStore>()
                            .CommitAsync();
                    })

This ensures that the ISession is only commited at the end of the shell scope, unless if SaveChangesAsync() is called explicitly, which is the case in some places, maybe not always for a good reason, sometimes just because done before the above pattern.

[jtkech]

@MikeAlhayek

As said

Okay, let me work on it this night.

I'm working on but I saw that are pushing changes.

[jtkech]

That's okay, you didn't change ContentDefinitionManager

[MikeAlhayek]

@jtkech sorry I just saw your message. I did some cleanup to get it ready. Anyway, I think I am done with my changes. Let me know once you are done please.

Thank you

[MikeAlhayek]

@jtkech I did another round of testing and see no issues. I am going to merge this PR.