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

Add InvalidatingCachingProvider and ResolvePathTemplateManager (ease of use and improved debugging support) #250

Closed
axefrog opened this issue Mar 12, 2015 · 6 comments

Comments

@axefrog
Copy link

axefrog commented Mar 12, 2015

You've got a great library here. I see there is an attempt to provide some easy methods, but I don't think they actually cover a really common use case - specify a file path and model and have RazorEngine automatically compile and cache the template and reload it if the source file changes.

Specifying the template key, deciding whether I want to Run, RunCompile or Compile, setting a template manager, reference resolver, etc. - these are all implementation details that would benefit from being buried under the hood for most people. I'm not saying don't have them as options like you do right now, but in most cases I just want to specify a file path and a model and have the thing render.

Your compilation and caching can automatically make decisions with regards to the model type that is passed in. That's another decision that you can hide from the user, because in most cases, they won't care. A few boolean option flags on the default static instance should provide most configuration decisions and can be set in a static constructor at the start.

// Make it easy to set common options
Engine.Razor.Config.ViewPath = Path.Combine(AppDomain.CurrentDomain.BaseDirectory, "Views");
Engine.Razor.Config.EnableLayouts = true;
Engine.Razor.Config.EnableSomeOtherOption = false;

// Make it easy to render a template and not have to worry about how it's done
string html = Engine.Razor.Render("Home/Index.cshtml", new { Title = "Hello" });

I like what you've done; if you could just make the primary use case easy then it would be perfect.

p.s. While you're here - without having to make my own implementation of different things, is there a way to get it to recompile a template that is already cached? For development, I just want it to reload the template on every request, because I'm frequently updating the template, and so caching is not wanted.

@matthid
Copy link
Collaborator

matthid commented Mar 12, 2015

The short version is I don't think it is as easy as you might assume (I'm speaking of caching). For giving a file instead of a string I think we can easily provide an extension method.

You probably want #232 in your development phase.

I will come up with a more detailed answer later.

@matthid
Copy link
Collaborator

matthid commented Mar 12, 2015

Ok as promised here more details:

First: I understand where you are coming from and see this as value feedback. However especially the caching is a really hard and ugly topic. I will try to break it down into pieces.

One thing that makes RazorEngine so useful (over other engines) is the incredible performance once a template is parsed (read compiled and cached), which is basically a simple method call!
However one thing that hits us hard is:

There is no way to unload an individual assembly without unloading all of the application domains that contain it. Even if the assembly goes out of scope, the actual assembly file will remain loaded until all application domains that contain it are unloaded.

When I started maintaining the library I looked through the issue list and there were a lot open issues directly related to this or indirectly through the way RazorEngine "solved" it previously. That's when I redesigned the API in a way it would throw stones at you as soon as you start doing things resulting into a memory leak or weird behavior, while at the same time giving you all the necessary APIs to fix the issue (I'm not saying this is all perfect, just that everything is here to solve things). Now there are rarely bug reports and only feature requests (like this thread).

With the above things in mind let me say you: There is no one fits all solution here. The only solution for a general caching strategy would be to spin up our own AppDomains, unload it once "they are full" and start again. Now there are again several possibilities: Do we create a AppDomain for each template (so we don't unload everything only because a single template was edited) or one for all templates or something in between? What is considered as full?

The other thing is the performance impact of this (see http://stackoverflow.com/questions/1144459/what-is-the-minimum-cross-appdomain-communication-performance-penalty). So its probably a better idea to recycle your whole application and use "raw" RazorEngine if you need the performance. But that cannot be done by us obviously.

That all being said: The new API with working isolation is already a step forward to be able to implement one of the mentioned solutions, because the RazorEngine interface is already designed to work across AppDomains. So I'm open to contributions regarding proper caching, but IMHO they always will be short term solution, because I think the main selling point of RazorEngine is speed. I would also not allow any such caching to be the default as users should first fail and think about the problem (maybe they can somehow live without it or know their app gets restarted from time to time and it's ok to just load everything, like #232).

Hope this helps.

PS: currently RazorEngine has a build-in memory leak by design, if you always use a different template name. Maybe we should limit the number of templates to fail early when this is miss-used.

Related: #244, #232

@axefrog
Copy link
Author

axefrog commented Mar 12, 2015

Thanks for the detailed reply. The concerns are perfectly understandable. Perhaps it would be worth providing some kind of built-in configuration for use during development, whereby the developer accepts that recompilation will cause a slow memory leak? I initially called AddTemplate, followed by Compile in my constructor, then Run in my main renderer method. That worked fine. What would have eased development considerably was simply being able to invalidate the cache on demand. It would be easy to remove that line once development is complete. As a slightly more robust solution, it would also be easy to place it inside an #if DEBUG block, with a #warning This will leak if called too often. Do not deploy in debug mode. Etc.

@matthid
Copy link
Collaborator

matthid commented Mar 12, 2015

My concern is: If I add such an API to invalidate the cache (which is trivial in itself) people will use it, and complain when it breaks (which is perfectly understandable). That's the dilemma. IMHO doing such things should be possible but hurt a bit. How about doing something like this:

#if Debug
#warning ...
// The InvalidatingCachingProvider class should have a huge warning sign.
var caching = new InvalidatingCachingProvider();
config.CachingProvider = caching
config.TemplateManager = new DebugReloadResolvePathTemplateManager(caching);
#else
config.TemplateManager = new ResolvePathTemplateManager()
#endif

If both those classes are implemented correctly they will solve all your problems, including being able to specify file-paths directly as name parameter of Run RunCompile and Compile.
DebugReloadResolvePathTemplateManager would invalidate the cache (or rebuild it whatever is preferred).
I'm open to include such implementations in the main library. It was one of the design goals of the new API to make it easier to contribute such interface implementations after all.

@matthid
Copy link
Collaborator

matthid commented Mar 12, 2015

The other thing is, would you have found them if they were already included in RazorEngine? If you look into the quickstart there already is a TemplateManager implementation given and if you change the Resolve method to

    public ITemplateSource Resolve(ITemplateKey key)
    {
        var name = Path.GetFullPath(key.Name)
        return new LoadedTemplateSource(name, name);
    }

You already have the resolution you wanted in your opening post, so there is no need to add further Extension methods (what I initially thought). We could include the implementation in the core RazorEngine library, but because the implementation is so simple I wanted to let the users decide how they want to resolve their templates (what if they want to search in another folder and not the current directory...) and only included a implementation that does what RazorEngine did in previous versions (backwards compat)... Maybe I was wrong on that point though.

@matthid matthid changed the title Most common use case is too complicated Add InvalidatingCachingProvider and ResolvePathTemplateManager (easy of use and improved debugging support) Apr 22, 2015
@matthid matthid changed the title Add InvalidatingCachingProvider and ResolvePathTemplateManager (easy of use and improved debugging support) Add InvalidatingCachingProvider and ResolvePathTemplateManager (ease of use and improved debugging support) Apr 22, 2015
matthid added a commit that referenced this issue May 1, 2015
add InvalidatingCachingProvider, ResolvePathTemplateManager and WatchingResolvePathTemplateManager, fixes #250.
Switched to Apache 2: #190
Missing feature (compared to 3.6): Configuration fallback to XML.
@matthid
Copy link
Collaborator

matthid commented May 2, 2015

We have this now in 3.7.0. The DebugReloadResolvePathTemplateManager is called WatchingResolvePathTemplateManager

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants