Working on caching and tokens

[jtkech]

No description provided.

[jtkech]

As a reminder

JObject and then Entity is not thread safe. So e.g this is not thread safe to mutate the Properties of the shared cached ISite.

Update: and idem for all other fields whose assignment are not thread safe, this for object that are cached and shared at the tenant level and that can be mutated. I will need some cloning, immutable collections ... as done in #3930 for ContentDefinitionManager.

[jtkech]

Came back too late to make some progress so i just added some comments.

So i try to use the following rules but i still have a little concern with point 2.

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

In point 2., when cancelling the token after storing data, another thread may get a new valid token and read data that are not yet updated. Maybe that's why before we were also updating the cache here, but this would not solve the problem in a distributed env and i prefer to always update the cahe with the actual persisted data, e.g by reading again the database.

So, at some point i added a CommitAsync() but this ends the current transaction so when we do multiple saves in the same scope, it says that the object is already tied to another transaction (or something like this).

So, when a token is tied to data persisted in the database, one idea would be to have a token signal that is only sent at the end of the scope and executed after the session is commited. Knowing that now we can easily register a BeforeDispose() callback. I will try this tomorrow.

Update: okay seems to work well with the following, beautiful ;)

        Session.Save(existing);

        // Invalidates the cache at the end of the transaction.
        ShellScope.RegisterBeforeDispose(scope =>
        {
            _signal.SignalToken(SiteCacheKey);
            return Task.CompletedTask;
        });

[jtkech]

So, here the main goal was to update services that were already using local caches and signal tokens so that we will be able to keep them in sync by using a distributed signal service.

I didn't change here the other services that will need some adaptations but that don't already use the signal service, we will see when re-working on distibuted services. E.g AutorouteEntries where a local cached value invalidation is based on a ContentPartHandler.

Update: Better to 1st merge #3930 before finalizing and then merging this one. E.g in #3930 there is a more complete version of ContentDefinitionManager that i made async.

[sebastienros]

Why? Is a User object used in concurrent threads?

[jtkech]

As i remember, there are some places where we mutate it and i checked that we act on a same shared instance.

Let me some time to retrieve an example

[jtkech]

E.g in UserStore.AddToRoleAsync() line 332

        ((User)user).RoleNames.Add(roleName);

That becomes here

        ((User)user).RoleNames = ((User)user).RoleNames.Add(roleName);

[sebastienros]

But only one thread accesses this user instance, so the list is thread-safe.

[jtkech]

Yes normally but just tried 2 concurrent editing (by using a break point) of the same user, i could see that the 2 concurrent thread are mutating the same user instance and then its roles collection.

The other problem is when we are using / enumerating on e.g roleNames while mutating it.

[sebastienros]

Interesting. Where are we mutating and iterating at the same time, if not on two different thread.

just tried 2 concurrent editing

This is worrying. Can you explain how that happens?

[sebastienros]

UserManager is scoped, it resolves UserStore which is scoped too, which uses Session to load a user. In the end I don't understand how two thread would access the same user instance, unless we have some wrong parallel code.

[jtkech]

Okay let me check again

[jtkech]

Oh, just retried, you're right there are 2 different instances, i don't know what i did, maybe too tired ;) These were some of the last changes i did as for others singletons and without doing the above test, indeed there are all scoped services, i'm stupid.

Thanks, i will revert all the changes around user and roles.

[jtkech]

So, i don't know why i did here the same changes as for other services but using a singleton cache.

Sorry, i reverted the changes related to users and roles.

[jtkech]

@sebastienros for infos

Okay when a GET is done before a LOAD in the same scope, in the GET for caching we do a session Detach() after the query. But because a GET may be also done after a LOAD in the same scope, we also need to do a session Detach() before the query. This is what i did by detaching the document we cache at the scope level when doing a LOAD (same cache as you did for AdminMenuService).

So i did an ISessionHelper registered as a scoped service to make our code simpler. This service has a LoadForUpdateAsync() and a GetForCachingAsync() methods. The 1st one has a scoped cache so that the 2nd one can check if a given document type has been loaded and then detaches it before a query.

This is useful particularly for the SiteService that is a singleton and then that can't hold a scoped cache when we do a LOAD. Here i needed to register and use a specific scoped cache, this would be no more necessary with the above helper.

I already used this heper but right now only for the LayerService, i will continue to use it for other services, but let me know if it is not okay (wrong way, bad names ...).

[jtkech]

Close and reopen to restart appveyor building

[jtkech]

Reopen to restart appveyor building

[jtkech]

@sebastienros i'm ready with this one, for the types / parts definitions i tried it with both file and database definition stores. If going to be merged, would be good to do it in pair with #4164.