More async calls

[jtkech]

in progress ...

[jtkech]

Not cool, IRouter.GetVirtualPath() is not async, so in AutorouteRoute.GetVirtualPath() there is this synchronous execution (.Result at the end) that uses e.g contentManager.GetAsync().

var displayRouteData = GetContentItemDisplayRoutes(context.HttpContext, contentItemId).Result;

Called to generate paths e.g through an urlHelper used in a view e.g to list all the blog posts.

In 3.0 when using the endpoint system another kind of urlHelper using a linkGenerator will be used, but with the same async problem. I will try to find a way to overcome this.

The problem is that we have a custom router for all autoroutes and we might have an autoroute for many content items, so we can't fully initialize this router once because we would have to get all content items to grab their ContentItemMetadata.DisplayRouteValues. So i'm thinking of some global route values shared by all content items unless their contentItemId, i will try it tomorrow.

[jtkech]

So, i refactored the home and autoroute features to prevent some non async code executions.

• Because of a previous PR SiteSettings.HomeRoute is initialized to a non null value but this doesn't prevent another ISiteService implementation to return another type of ISite with a null HomeRoute. So here i added some null checks but there are other places without null checks, let me know.

  But i'm fine with the rule that any ISiteService would need to provide an ISite where HomeRoute is never null. If so, maybe i could remove my additional null checks as in other places, let me know.

• I thought about removing HomeRoute and just keep Autoroute, or making Autoroute dependent of HomeRoute and so on. But finally i kept HomeRoute and Autoroute and let them quite independent. Autoroute being a way to manage the home route setting, indeed it needs a feature that uses it but not forcibly HomeRoute. Then HomeRoute uses the home route setting but is not aware of Autoroute.

• I fixed some config ordering so that e.g the list of blog posts renders the right link for a blog post that is set as the homepage.

• I also fixed the fact that when we publish an item with another autoroute path, before the previous path was not removed from one of the 2 mapping tables.

[jtkech]

Just tried to make the scripting engines async, but i reverted my changes because the Jint.Engine doesn't have an ExecuteAsync() method and we can't provide to it GlobalMethod that are async.

[jtkech]

Most of the remaining non async calls are

• In startup configurations, that's ok.

• In scripting methods, maybe good that the Jint.Engine exposes an ExecuteAsync() so that we could provide to it async methods.

• In TaxonomyIndex.Map() using the new _contentDefinitionManager.GetTypeDefinitionAsync(). Maybe good that YesSql supports MapAsync() and ReduceAsync() for indexes.

  Is it a problem if an index Map() uses itself the session? Here through GetTypeDefinitionAsync() if not already cached and if we are using the database vesion of the definition store.

[jtkech]

So, i made FileContentDefinitionStore thread safe by registering it as a tenant singleton and by using a ReaderWriterLockSlim to not lock readers if we are not writing the file.

But ReaderWriterLockSlim can't be used in an async context so here FileContentDefinitionStore now runs synchronously as before, knowing that definitions are cached.

[jtkech]

@sebastienros nothing urgent for me but here my little roadmap for this week (unless the last point).

• I think this one is ready, more async calls and some fixes (i could retrieve them if needed).

• A PR to update the usages of caching + tokens, not only to adapt them for distributed services, but because here i saw other things (updated here for 2 services) and i defined some rules e.g:

  • Always get a new token before consuming / using data.
  • Cancel the token after storing data (and committing if database).
  • Update the cache only when reading data from the store (not when storing).
  • Use the token (can be cancelled by another instance) to invalidate the local cache.
     

  Here i needed to apply these rules e.g for ContentDefinitionManager but i also made all its usages async, so better to 1st merge what is already done before continuing this work on another PR.

• Then, merge dev in 3.0 and solve conflicts (i fixed the home route issue with preview9).

• That would be a good base for re-working on distributed services.

[jtkech]

@sebastienros

• Yes, making the content definition manager async hides other things but i think this PR is quite important. I made around 20 other calls async knowing that for some i kept the non async version for compatibility, this around recipes, razor pages, shapes, templates ... and fixed some issues.

• Yes, home route and autoroutes are not the same in 3.0 but here i introduced AutorouteOptions and HomeRouteFeature to make some code simpler that i planned to use in 3.0.

• Do you want that i revert back the definition manager to be non async? So that i could create its final version in the caching PR Working on caching and tokens #4001, then this PR will be much more light, and both PRs will not depend on each other.

• But i need a confirmation, when using the database store, it was making sense for me to do session stuff async to prevent some db locks. Maybe even if we cache the definitions at the tenant level (currently it is at the scope level, only new built types / parts are cached at the tenant level). Note: and with distributed services where definitions could be invalidated ...

• Anyway the content definition manager needs to be fixed, a type / part can be built on a stale definition, there are some race conditions on concurrent mutations, if we keep it non async i can do that in the caching branch as i did for other things as ISite, AdminMenu and so on.

[sebastienros]

I like it like that. Merge.

[sebastienros]

once the tests pass obviously

[sebastienros]

Also I am very surprised that the types were not cached correctly (as singleton), that should improve perf a lot.

[sebastienros]

Also you can remove the non async Permission provider.

[jtkech]

Okay thanks, will do but i want to redo some minimum tests.

Also i was just removing the IScriptingEngine.EvaluateAsync() (not committed yet). I did it async just for the FilesScriptEngine that is not frequently used (maybe only on setup).

So knowing that right now the JavaScriptEngine can't be async, do you think it's better that i continue to remove the related async calls?

[jtkech]

Also you can remove the non async Permission provider.

Only 2 uses the async version, so around 50 providers will return a Task.FromResult, that's ok?

Also I am very surprised that the types were not cached correctly (as singleton), that should improve perf a lot.

Here i reverted all the changes around the ContentDefinitionManager so it is as before. This is in the caching branch that i committed the final version that caches the definitions at the tenant level.

Note: currently the definitions are cached at the scope level but each time we build a type / part (using the definitions) it is cached at the tenant level. The final version, committed in the caching branch #4001, also caches the base definitions (from which we build types / parts) at the tenant level.