OrchardCMS / OrchardCore

Orchard Core is an open-source modular and multi-tenant application framework built with ASP.NET Core, and a content management system (CMS) built on top of that framework.
https://orchardcore.net
BSD 3-Clause "New" or "Revised" License
7.36k stars 2.37k forks source link

IVolatileDocumentManager fails to return updated document during same request #16014

Open deanmarcussen opened 4 months ago

deanmarcussen commented 4 months ago

When using the IVolatileDocumentManager to cache documents, updates to said document are commited at the end of the scope.

What can occur, is that after updating an item, you also want to retrieve it in the same request, to perform some other operation.

This can fail, unless the method used to retrieve the item is GetOrCreateMutableAsync

the UpdateAsync method, places the document in the ShellScope, and if you later make another update to said document, this updated version is retrieved.

However, if you retrieve said document with GetOrCreateImmutableAsync, the ShellScope is not checkedl, so you get the old document back.

Obviously the fix for this it to call GetOrCreateMutableAsync when you require it. This is actually very hard, because it is hard to know when to call it, as often these are quite disconnected handlers, that are working in coordination.

One solution is to prefix calls with a check on the shell scope to see of the relevant document is there... but that's just a bit of a hack (and quite frankly involves knowing way too much about the IVolatileDocumentManager)

I think it probably needs to check for a volatile cached object in GetOrCreateImmutableAsync as well, but you may end up with a document that is at that point mutable. (noone will know, but maybe it will also cause odd behaviour)

sebastienros commented 4 months ago

I'd like to suggest to remove the whole DocumentManager concept, but also I tried once and then realized why it was not possible without losing something else. Don't recall what, should try again.

Thinking about delegating the distributed consistency issue to an underlying implementation (see hybrid cache in aspnetcore 9). If anyone has pointers to the DocumentManager goals I'd like to refresh my memory.

deanmarcussen commented 4 months ago

The DocumentManager concept originally rose because we wanted to memory cache individual YesSql documents, e.g. TemplatesDocument is a good simple example.

It then grew to support a distributing caching arrangment.

And it became more complex, as we threw things like Autoroute at it, and more complex documents, than the simple template example.

For me, the concept is generally good, we need / want a way of caching those individual documents, like templates, so they are rendering from memory cache pre request, not retrieving from the database per request.

However, where it significantly falls over / has issues / creates unneeded / unwanted complexity, is with this idea that got introduced at some point (to fix an unanticipated problem), of retrieving a mutable or immutable document. It introduces to much complexity about consumer intent, and means this ide of GetMutable and GetImmutable polutes the consuming code.

I suggest to JT after c# records came out, that we look at using them, to create more immutable documents, so we no longer have this pollution, but we never went anywhere with investigating it. Ultimately because collections on records are not automatically immutable, it probably doesn't solve the issue, but it still feels like a concept worth exploring,

ns8482e commented 2 months ago

I believe DocumentManager is used to load once and keep and in memory scenarios, for frequently consumed data ie, for Settings, Shell, Templates etc

Changed document is saved to db at the end of the request