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

Re-loading of an updated "Include"/partial template section? #515

Open
marekvse opened this issue Jan 10, 2023 · 12 comments
Open

Re-loading of an updated "Include"/partial template section? #515

marekvse opened this issue Jan 10, 2023 · 12 comments

Comments

@marekvse
Copy link

Describe the bug
I have a multi-level templates where the main layout has HEADE and FOOTER sections and the main layout is then used by an actual email template.

When I update the content of the template representing the FOOTER for example and even after I remove the template from the LookupCache as well as remove all the dependent templates from the LookupCache, too. Yet, opening any of the dependent layouts using the FOOTER section, they do not show the update because the callback to load the HTML for the section (from the database in my case) is never called again after it has been called just fine the first time.

I'm loading the templates from the database by their ID using the RazorLightEngineBuilder.UserProject(callbackFunction) and that works fine the first time and calls the "callbackFunction" as many times as it needs to load all the dependent layouts. But it does not call the callbackFunction again when I update an included layout and despite removing the cached items.

How do I force it to reload the source html/Razor definition again for those templates that are not in the LookupCache anymore?

Expected behavior
The callback function defined in RazorLightEngineBuilder.UserProject(...) should be called again if that particular (section) template is not in the Cache anymore and a layout that uses that section is loaded.

@jzabroski
Copy link
Collaborator

jzabroski commented Jan 16, 2023

Clearing the cache and compilation are separate tasks. It sounds like you want a CacheRefresh feature that clears the cache and re-compiles the cleared keys?

Updating the layout, e.g. on disk, is not sufficient.

@marekvse
Copy link
Author

That's basically right @jzabroski . Except I would not want to clear the whole compilation cache every time I update a single include. I would like to clear the cache selectively, supplying the IDs of the templates for which I want the compilation cache cleared. Then, it should add those that need to be compiled as required, as it does right now. I just want to be able to say: OK, I updated template no. 17, so clear the compilation cache for that one, but I also know that templates 23, 24, 41 and 50 directly or indirectly depend on it so clear the cache for those as well, if any of them have been compiled and stored in the compilation cache.
Clearing the compilation cache wholesale would be a "poor man's" solution as I would prefer to keep all the unaffected templates that I may want to view compiled and not waste time recompiling those, but at this point, I'd even welcome that solution. It would still be better than restarting the whole application.

@jzabroski
Copy link
Collaborator

jzabroski commented Jan 17, 2023

To repeat back to you what I heard, you want something like:

public interface ITemplateCacheReload {
  Task<ISet<string>> SnapshotKeys(); // take a snapshot of keys. debatable this is necessary. just brainstorming interface.
  Task CacheRefresh(ISet<string> keys); // supply a snapshot of keys. refresh would call renderCallback.
}

Clearing the whole compilation cache, while likely never to be used, could then be achieved via:

await CacheRefresh(await SnapshotKeys());

With this API, you would be responsible for tracking the dependency tree across templates yourself. I don't think we track that currently. Hence why I suggested clearing the whole compilation cache. Of course if you know the tree yourself, e.g., because you are storing the dependencies in a database as a nested set, then this is mostly trivial.

EDIT: Actually, this won't work right - there will be sequential coupling in the ISet<string> keys parameter. Unless we know the DAG and refresh bottom-up, we can't guarantee write-through cache coherence.

@marekvse
Copy link
Author

Yep, that's look all right. I just need something to clear the cache - per template or at worst completely.

I sure am tracking all the dependencies in the database so I know exactly what other templates I need to clear from the cache.
I've basically build an email template editor where the user can easily switch between templates and the user may want edit the base template, like the footer, which is refrerenced by the main layout, which is then used in specific email templates. I even use more depths than that in the software I built this editor for, but that is enough to demonstrate the use case. If the user cannot see the change in the footer in this example, they will think something did not work. And the only way for them to see it is for me or the application / server admin to restart the app, which surely is not the best for usability :).

Re your EDIT: I'm not sure what you are specifically talking about there as I do not know the source code that intimately, but thre surely needs to be a way to clear the compilation cache programmatically.

@jzabroski
Copy link
Collaborator

I meant that the interface as a set of keys is a leaky/incorrect abstraction since the issue you are trying to address is fundamentally a dag, not a set. Getting all snapshot keys if therefore not helpful even. So the minimal viable product would simply be:

Task RefreshCachedTemplate(string key);

You would then need to sequentially call await RefreshCachedTemplae(key) bottom-up.
The cost of this is that others who use this will not understand that it won't work well unless they track the DAG themselves.

@marekvse
Copy link
Author

marekvse commented Jan 17, 2023

OK, had to look up what is a DAG :). I did not go to UNI for programming so I sometimes am not aware of these generic concepts...

Anyway

Task RefreshCachedTemplate(string key);

sounds fine to me. Although I would prefer

Task RemoveCachedTemplate(string key); 

because at the point the template (let's say the footer from my earlier example) is edited I just need to make sure that its previous compiled version is removed from the cache, not that it or any of the higher level templates that depend on it, are recompiled right then. The user may not even go back to the previously opened template that depends on it even if they did have it open before. The "Remove..." function would allow me to simply make sure that I removed all the cached templates in the tree of dependencies that have been compiled into the cache and whether or not they get re-compiled and put back into the cache later is going to be taken care of by the already existing code I believe, where if a required Include is not found in the cache it is requested by the component from the code that uses it and it gets compiled then.

@marekvse
Copy link
Author

marekvse commented Jan 17, 2023

Oh, and as for the cost, I think there are lots of things that may be unclear in any component out there. Naturally, you have to read up on it or simply look into the IntelliSense description of the function.

I think that much more confusing, for example, is the ability to remove the templates from LookupCache which does not currently remove it from the compilation cache. I probably do not understand what the difference is there, but I for one would expect the action of removing something from LookupCache (or whatever other name it could have been called) would do what it says, which is "removing the template from the cache" and that, in my mind, should include automatically removing the template from all the caches that the component may be maintaining, so also from the compilation cache.

So in fact, if the above made sense, there would be no special need for any new function like RemoveCachedTemplate if the Remove function of the LookupCache class did this by default. In that case, I would not even have to change anything in my own code except just updating the version of RazorLight :), bacause that is the behaviour I was in fact expecting.

So unless there is a reason to keep the remove functions separate for those two types of caches, I'd vote for just extending the functionality of the existing remove function to make sure the cached compiled template is removed as well.

@jzabroski
Copy link
Collaborator

jzabroski commented Jan 17, 2023

I think that much more confusing, for example, is the ability to remove the templates from LookupCache which does not currently remove it from the compilation cache.

Just a guess: I think it was never intended to be 'removed' from the cache. One immediate workaround is to de-allocate the EngineHandler, call the GC, and instantiate a new EngineHandler. Calling the GC is useful as you may have items in third generation storage, that might not be immediately finalized.

In looking at the code... it does seem a bit of an oversight.

The two types of caches already exist today:

  1. RazorTemplateCompiler, inside CompileAsync, caches a CompiledTemplateDescriptor
    1. This is the 'first tier' of the cache.
    2. It is designed to avoid CPU overhead.
  2. EngineHandler caches the output of RazorTemplateCompiler.CompileAsync
    1. This is the 'second tier' of the cache.
    2. It is designed to avoid non-CPU overhead/resource constraints.
    3. This is the cache interface you are removing from
    4. It is designed to allow multiple cache providers, including a file system ,which would relieve memory pressure/memory resource constraints as well as open the door to using a content delivery network over fast broadband connections like Amazon S3. An example would be generating large printable HTML documents that might be 200 megs in size.

public Task<CompiledTemplateDescriptor> CompileAsync(string templateKey)
{
if (templateKey == null)
{
throw new ArgumentNullException(nameof(templateKey));
}
// Attempt to lookup the cache entry using the passed in key. This will succeed if the key is already
// normalized and a cache entry exists.
if (_cache.TryGetValue(templateKey, out Task<CompiledTemplateDescriptor> cachedResult))
{
return cachedResult;
}
string normalizedPath = GetNormalizedKey(templateKey);
if (_cache.TryGetValue(normalizedPath, out cachedResult))
{
return cachedResult;
}
// Entry does not exist. Attempt to create one.
cachedResult = OnCacheMissAsync(templateKey);
return cachedResult;
}

public async Task<ITemplatePage> CompileTemplateAsync(string key)
{
ITemplatePage templatePage = null;
if (IsCachingEnabled)
{
var cacheLookupResult = Cache.RetrieveTemplate(key);
if (cacheLookupResult.Success)
{
templatePage = cacheLookupResult.Template.TemplatePageFactory();
}
}
if(templatePage == null)
{
CompiledTemplateDescriptor templateDescriptor = await Compiler.CompileAsync(key);
Func<ITemplatePage> templateFactory = FactoryProvider.CreateFactory(templateDescriptor);
if(IsCachingEnabled) {
Cache.CacheTemplate(
key,
templateFactory,
templateDescriptor.ExpirationToken);
}
templatePage = templateFactory();
}
templatePage.DisableEncoding = Options.DisableEncoding ?? false;
return templatePage;
}

@jzabroski
Copy link
Collaborator

I think the takeaway is that the EngineHandler should handle template cache invalidation and forced cache eviction. The other cache should probably only be used in the sense of relieving memory pressure from a cache eviction standpont.

Do you agree?

@marekvse
Copy link
Author

Yes, that sounds correct to me.

Thank you for looking into this. And thank you for the two snippets above. That should help me to make the necessary changes to implement the behavior I need.

Do you think that is also something that may get implemented in the near future? My software won't be out for a while so if I knew there is a good chance this will get implemented by the original author(s) I wouldn't have to spend time fixing it for myself, where my fix may be sub-optimal.

@jzabroski
Copy link
Collaborator

I would love a PR. I am pretty busy these days with a 5 month old baby boy.

@marekvse
Copy link
Author

OK, I'll try to do that when I get around trying to fix it. However I cannot say that I'll be very confident in offering the best solution for a public component. As soon as it starts working for me I need to be on other things.
Thanks again! :)

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

No branches or pull requests

2 participants