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

Template temporary dll deletion fails with System.UnauthorizedAccessException and files are left on disk #244

Closed
rubu opened this issue Mar 5, 2015 · 44 comments · Fixed by #254

Comments

@rubu
Copy link

rubu commented Mar 5, 2015

We've been using Razor for hosting a web interface for a service successfully so far, but since upgrading from 3.3.0 to 3.6.1 we've run into an issue - we get a ton of System.UnauthorizedAccessException at the end of the program when the finalizers are called. Basically Razor complains about not being able to delete files like C:\Users\user\AppData\Local\Temp\RazorEngine_becmcjic.mja\CompiledRazorTemplates.Dynamic.RazorEngine_69ed0018c26e44dca13eba07dcb6bfd6.dll, this happens in the CompilationData finalizer:

mscorlib.dll!System.IO.__Error.WinIOError(int errorCode, string maybeFullPath) + 0xd8 bytes 
mscorlib.dll!System.IO.File.InternalDelete(string path, bool checkHost) + 0xd6 bytes    
RazorEngine.dll!RazorEngine.Compilation.CompilationData.DeleteAll() + 0x10f bytes   
RazorEngine.dll!RazorEngine.Compilation.CompilationData.Dispose(bool disposing) + 0x3b bytes    
RazorEngine.dll!RazorEngine.Compilation.CompilationData.Finalize() + 0x3d bytes 

Before the upgrade no such issues were present. The biggest change during the upgrade was that now we precompile all the templates before firing up the web interface:

TemplateServiceConfiguration templateConfig = new TemplateServiceConfiguration();
templateConfig.TemplateManager = new DelegateTemplateManager(name =>
{
    string resourcePath = string.Format(viewPathTemplate, name);
    var stream = Assembly.GetExecutingAssembly().GetManifestResourceStream(resourcePath);
    using (StreamReader reader = new StreamReader(stream))
    {
        return reader.ReadToEnd();
    }
});
RazorEngineService.Create((ITemplateServiceConfiguration)templateConfig);
RazorEngineServiceExtensions.Compile(razorEngineService, "Layout.cshtml");

I've narrowed it down to a standalone sample:

using System;
using RazorEngine;
using RazorEngine.Templating;
using RazorEngine.Configuration;
using System.Reflection;
using System.IO;

namespace RazorTest
{
    class Program
    {
        static void Main(string[] args)
        {
            TemplateServiceConfiguration templateConfig = new TemplateServiceConfiguration();
            templateConfig.TemplateManager = new DelegateTemplateManager(name =>
            {
                return "<html><head></head><body><label>Hello World!</label></body></html>";
            });
            var razorEngineService = RazorEngineService.Create((ITemplateServiceConfiguration)templateConfig);
            RazorEngineServiceExtensions.Compile(razorEngineService, "Index.cshtml");
         }
    }
}

This triggers the System.UnauthorizedAccessException on exit.

Since previously Razor was working quite nice already out of the box I actually haven't been digging very deep into the internals and have no idea how to approach this issue. Can anyone provide any suggestions or tell what additional information from my side would help to understand the issue? From my point of view, if the dll's are still loaded by the engine of course it won't be able to delete them, but that is just my two cents. Is there any way to make the engine unload everything?

@matthid
Copy link
Collaborator

matthid commented Mar 5, 2015

I cannot repro this. If you look into CompilationData (https://github.com/Antaris/RazorEngine/blob/master/src/source/RazorEngine.Core/Compilation/CompilationData.cs#L52) I cannot see how a UnauthorizedAccessException would slip through there... Maybe you have set your Visual Studio to hold on triggered exceptions (even if they get handled)?

Yes we have a problem here, we can't delete loaded dll files and there is nothing we can do, we try it anyway because we actually can cleanup on non-windows platforms.

If you need proper cleanup in your application the only thing you can do is use Isolation and delete the files manually after shutting down the AppDomain (you need to communicate the folders manually). There is currently no API which helps you to implement this and you probably need your own ICachingProvider implementation which communicates the files to the master AppDomain.

For performance reasons it might even be better to create your own AppDomain instead of using the isolation API (so you can save the Serialization overhead)

@rubu
Copy link
Author

rubu commented Mar 5, 2015

@matthid , first thanks for the quick response. Since the deadline for the project is close I was maybe too pushy to get some info, sorry for that.

I made an incorrect formulation of the issue, you are right - the exceptions as such do not cause issues (I caught them because I had the debugging settings set to catch exceptions on being thrown, as you correctly stated, they are caught in Razor code). What I was mostly concerned about was the disk trashing since I had no way to keep track of the files created.

Thanks for the explanation, I will look into making my own caching provider to clean up. Is there any sense in sticking with 3.3.0 instead of solving this? In the 3.3.0 code I used Compile().Run() each time and reading the documentation there should be a performance increase using precompiled templates and also I really like that I can catch parsing error on startup not on first use.

Many thanks for the information.

@matthid
Copy link
Collaborator

matthid commented Mar 5, 2015

Yes in 3.3.0 Assembly.Load(byte[]) was used and therefore the file was not locked. However this means the generated assembly is loaded with Full Trust no matter what. So I figured having some temporary files is better than code escaping your sandbox because of this :)
Loading the files also improves debugging, so I would obviously recommend staying on the latest version.
If you find a nice solution to this problem a pull request is appreciated.

@rubu
Copy link
Author

rubu commented Mar 5, 2015

Well, I'm mainly a C++ developer, my C# is not that good and after reading about AppDomain on MSDN, serialization etc., I don't think I can provide a nice solution, so I would be thankful if you can provide me with any hints on how to solve this in a trivial way and then I'll just gladly wait when such functionality is implemented inside Razor.

  1. The first most trivial and straightforward approach is that I can just delete all Razor_* stuff in the temporary directory (but this could possibly conflict if there is any other application running Razor as well, bet then I'd just get an exception) when my application starts. I assume running Run() does not produce any additional dll files right? I produce them only when I do Compile() so that means that one launch of my application would produce a constant number of dll files which is fine then.

  2. Can I anyhow override the temporary directory with my own? Then at least I am sure I am deleting only my own files. DirectCompilerServiceBase (https://github.com/Antaris/RazorEngine/blob/master/src/source/RazorEngine.Core/Compilation/DirectCompilerServiceBase.cs#L81) seems to hardcode the path and I did not see a way to override this, is that correct?

  3. I had a look at making my own AppDomain, but as far as I understand it always needs uses serialization between the calling AppDomain and the receiving AppDomain (ok there was some stuff about implementing custom marshaling so I guess that is what you meant). But making a new caching provider based on the default one and passing in a new domain create by AppDomain.Create() resulted in System.Runtime.Serialization.SerializationException and I had no idea what I have to do to overcome this. If it is easy, you can provide me with hints, if not - don't waste your time since approach 1) seems quite straightforward.

And, aside from the context of the issue, great engine - I love the possibility to deploy a rich web interface from a service:)

@matthid
Copy link
Collaborator

matthid commented Mar 5, 2015

All your observations are correct. I think even deleting all Razor_ stuff is quite safe because everything that is actually used is locked anyway (so you can't delete currently used files from other applications by chance).

@rubu
Copy link
Author

rubu commented Mar 5, 2015

Thanks for the feedback:) Should I close this or leave it open as a feature request?

@matthid
Copy link
Collaborator

matthid commented Mar 5, 2015

I think we should leave it open as it certainly makes sense to think about it further and eventually fix this properly.

@ghost
Copy link

ghost commented Mar 10, 2015

+1. I don't yet have this issue, but was wondering about temp files ever since i upgraded from 3.4.x or something. As rubu mentioned above, having the ability to provide our own location where compiled assemblies are stored would be a big step forward. Having to poke around temp directories in file system is not really compelling, especially in production environment.

Having a way to propely dispose of loaded code is a must. I've had a nasty memory leak with previous version. Afterall it was a bug on my side of code, but nevertheless having a way to just destroy all loaded compiled code would be awesome. I've had some experience with loading stuff into a separate AppDomain (unloadable plugins) so I'll try to take a look at this when I have some spare time on my hands.

@matthid
Copy link
Collaborator

matthid commented Mar 12, 2015

Yeah everything you need is there, making the path configurable should not be too complicated. If you can live with the performance impact you can use the already provided Isolation API. You do not even need to change the path, but instead implement a ICachingProvider which communicates all paths to your main AppDomain which then deletes everything once the Isolated AppDomain is unloaded.

@matthid
Copy link
Collaborator

matthid commented Mar 12, 2015

Maybe there is another way: We could change the default ICachingProvider in such a way that it spins up a new 'cleanup' AppDomain and communicates all paths to cleanup with it. After the main AppDomain has been unloaded the new 'cleanup' AppDomain jumps in and deletes the remaining files. This would probably slow down application shutdown times, but it would probably be acceptable. If somebody wants to implement this, let me know.

@matthid
Copy link
Collaborator

matthid commented Mar 24, 2015

Ok I tried to implement the 'cleanup' AppDomain approach, however it seems to be a bit fragile. Can somebody please test that this works in a real life scenario: #254.

The only limitation is that you cannot use the default AppDomain otherwise it should just work.
If somebody can confirm that it works I will do a nuget release.

@rubu
Copy link
Author

rubu commented Mar 24, 2015

I could test it since I actually encountered the issue, but can you give a small note on how to do that (again sorry, I'm a bit new with C#/Nuget)? I just pull the sources, build a project and add the dlls instead of the nuget ones?

@matthid
Copy link
Collaborator

matthid commented Mar 24, 2015

I don't know if nuget has a build-in feature for that, but the following should work

git clone https://github.com/Antaris/RazorEngine.git
cd RazorEngine
git checkout cleanup_temp_files
./build.sh # when in git bash or just build.cmd

and then copy release/lib/net45/RazorEngine.dll to where nuget installed the package within your project (.nuget/RazorEngine-3.6.1/lib/net45). Just replace the existing dll and rebuild your project. Once you are done delete .nuget/RazorEngine-3.6.1 and nuget will restore the folder with the correct binaries.

@matthid
Copy link
Collaborator

matthid commented Mar 24, 2015

I could do a quick beta release as well if you feel more comfortable with that.

@rubu
Copy link
Author

rubu commented Mar 24, 2015

If it's not too hard that would certainly be better for me:)

@matthid
Copy link
Collaborator

matthid commented Mar 24, 2015

Please try https://www.nuget.org/packages/RazorEngine/3.6.3-beta1
Note that it should always just work however temporary files are ONLY deleted when you don't run RazorEngine in the default AppDomain.

@rubu
Copy link
Author

rubu commented Mar 24, 2015

Ok, so I installed the package and used the example snippet from the isolation API page to create an AppDomain (https://antaris.github.io/RazorEngine/Isolation.html) - I was able to compile all the templates but the files were still left on the disk and lots of first chance System.UnauthorizedAccessException instances figured in the debug output. However, testing this actually showed that this is not a way to go for me - I embed the cshtml files in the assembly, and then to ship them over from the main domain to the isolation domain I need to make them serializable, which seems more difficult than just deleting the files by hand.

@matthid
Copy link
Collaborator

matthid commented Mar 24, 2015

Thanks for testing. Did it work (ie at least as before) or crash for you? If it did work by just replacing that's a good sign (this is still all work in progress), even if not all files have been deleted...
The first chance exceptions don't really matter.

And for the new AppDomain better do not use the isolation snippet, that's for a sandboxed domain, and sandboxing does hurt to make it work. Instead try this:

        static int Main(string[] args)
        {
            if (AppDomain.CurrentDomain.IsDefaultAppDomain())
            {
                // RazorEngine cannot clean up from the default appdomain...
                Console.WriteLine("Switching to secound AppDomain, for RazorEngine...");
                AppDomainSetup adSetup = new AppDomainSetup();
                adSetup.ApplicationBase = AppDomain.CurrentDomain.SetupInformation.ApplicationBase;
                var current = AppDomain.CurrentDomain;
                // You only need to add strongnames when your appdomain is not a full trust environment.
                var strongNames = new StrongName[0];

                var domain = AppDomain.CreateDomain(
                    "MyMainDomain", null,
                    current.SetupInformation, new PermissionSet(PermissionState.Unrestricted),
                    strongNames);
                return domain.ExecuteAssembly(Assembly.GetExecutingAssembly().Location);
            }

            var template =
@"@helper Display(int price) {
    if (price == 0) {
        <text>free</text>
    } else {
        <text>@price</text>
    }
}
@Display(Model.MyPrice)";

            TemplateServiceConfiguration templateConfig = new TemplateServiceConfiguration();
            templateConfig.Debug = true;

            var razorEngineService = RazorEngineService.Create((ITemplateServiceConfiguration)templateConfig);
            razorEngineService.AddTemplate("Index.cshtml", template);
            razorEngineService.Compile("Index.cshtml");
            var result = razorEngineService.RunCompile("Index.cshtml", model: new { MyPrice = 0 });
            System.Diagnostics.Debug.Assert(result.Trim() == "free");
            return 0;
        }

I will make a new release 3.6.3-beta2 shortly which should do a better cleanup job.

@rubu
Copy link
Author

rubu commented Mar 24, 2015

It worked the same as before, no crashes or other newly introduced problems:) When you have a new beta I can test it with the provided snippet, it seems much better suited than what I am doing with the embedding of html.

@matthid
Copy link
Collaborator

matthid commented Mar 24, 2015

Now available in https://www.nuget.org/packages/RazorEngine/3.6.3. I disabled the cleanup code on mono because of https://bugzilla.xamarin.com/show_bug.cgi?id=28369. Documentation is here.

@Krashlog
Copy link

I noticed what appears to be a race condition with the cleanup code. We're using RazorEngine inside an ASP.Net Web Application to generate emails, and monitoring the Temp folder I can see the files being written with our AppPool identity (not the Defaut).

When I recycle the AppPool (either manually w/ IIsReset, or if it times out), most of the time the temp files won't be deleted. Sometimes, it does work as expected, though, and the files are gone after a recycle. Using ProcMon, I can see that when it fails, we're getting the "CANNOT DELETE" result (and not "ACCESS DENIED"), so my guess is that the files are still locked.

Maybe RazorEngine is trying to delete those files before releasing all locked resources, maybe a Thread.Sleep() would prevent / mitigate the risk?

@matthid
Copy link
Collaborator

matthid commented Mar 25, 2015

Sadly there is no bullet proof way to check if a AppDomain has been unloaded. What RazorEngine tries to do is here: https://github.com/Antaris/RazorEngine/blob/master/src/source/RazorEngine.Core/Compilation/CrossAppDomainCleanUp.cs#L314
This should be pretty robust...

What can happen is when you recycle the AppPool IIS tries to kill all threads with Abort (or it even kills the process). I'm not sure if RazorEngine can do anything in that situation. Any chance that you can get the stderr output of the process running RazorEngine? It is quite possible that it would help to explain what you are seeing.

@matthid
Copy link
Collaborator

matthid commented Mar 27, 2015

We now have #258 to load assemblies from memory instead of disk, this is a nice solution for small short lived applications...
You should read the warnings and use it like explained here: https://github.com/Antaris/RazorEngine#temporary-files

matthid added a commit that referenced this issue Mar 27, 2015
Use /nostdlib when we find a mscorlib (improves mono support)
Added `DisableTempFileLocking` to load assemblies in memory (to prevent temp file locking),
this is only recommended in a very limited amount of scenarios.
Please be aware of the consequences before using it.
Please read #244 for details.
Note that this can introduce memory leaks to your application.
@DavidRogersDev
Copy link

Hi, We've been using Razor Engine since before 3.3. We have been noticing some of the warning discussed here, but have not observed any problems yet.

In a nutshell, would we be better off reverting back to 3.3? We don't have any budget to look at this in depth (or even to upgrade to the new API) and are not keen on experimenting with loading new AppDomains - again, no budget and running in an Azure Webjob.

Is using 3.3 the safest option for us?
Thanks

@matthid
Copy link
Collaborator

matthid commented Apr 15, 2015

@DavidRogersDev You have several options at this point:

  • Regularly cleanup the temp folder of the user running the RazorEngine code (Maybe Azure does that already for you and it's therefore not even a problem? You can just disable the warning in this case, see below)
  • Revert to the old (pre 3.5) behavior by setting config.DisableTempFileLocking = true

You can just add this to the application startup code:

config.DisableTempFileLocking = true; // loads the files in-memory (gives the templates full-trust permissions)
config.CachingProvider = new DefaultCachingProvider(t => {}); //disables the warnings
// Use the config
Engine.Razor = RazorEngineService.Create(config); // new API
Razor.SetTemplateService(new TemplateService(config)); // legacy API

Just note that all the new Features like improved debugging or Isolation will not work. But you wouldn't have them by staying on an old version either.

Of course you can stay on the old version, but I do not recommend it. If there are blocking issues which stop you from upgrading we should address them here.

@matthid
Copy link
Collaborator

matthid commented Apr 15, 2015

One more thing to note is:
If the API is properly used we are not talking about huge traffic here:
We write about 20kb per template (for big templates) in the temp directory, so if you have a long lived app (running for a month) with 100 templates you will have 2mb written to the temp directory every month (whenever the app is restarted).
We trigger the warning just in case you have a use-case which dynamically generates templates for example (and even then you need to recompile a lot to start noticing). You will notice that you have a problem if the warnings don't disappear after accessing all templates at least once.
So maybe you don't even need to take care about this amount of data leakage at all.

@niemyjski
Copy link

We just upgraded from 3.6.6 to 3.7.0 and noticed this in our azure web job logs. Is this normal behavior?

[06/04/2015 15:17:37 > f42223: ERR ] RazorEngine: We can't cleanup temp files if you use RazorEngine on the default Appdomain.
[06/04/2015 15:17:37 > f42223: ERR ] Create a new AppDomain and use RazorEngine from there.
[06/04/2015 15:17:37 > f42223: ERR ] Read the quickstart or https://github.com/Antaris/RazorEngine/issues/244 for details!
[06/04/2015 15:17:37 > f42223: ERR ] You can ignore this and all following 'Please clean ... manually' messages if you are using DisableTempFileLocking, which is not recommended.
[06/04/2015 15:17:37 > f42223: ERR ] Please clean 'D:\local\Temp\RazorEngine_karso4nc.pzm' manually!
[06/04/2015 15:17:37 > f42223: ERR ] Please clean 'D:\local\Temp\RazorEngine_33kstmsp.sv5' manually!
[06/04/2015 15:17:38 > f42223: ERR ] Please clean 'D:\local\Temp\RazorEngine_zldvadou.zau' manually!
[06/04/2015 15:17:38 > f42223: ERR ] Please clean 'D:\local\Temp\RazorEngine_ep41io3n.xxx' manually!

@matthid
Copy link
Collaborator

matthid commented Jun 4, 2015

Here is a short summary of your options:

  • Do it manually:
    • Clean the temp folder regularly from time to time (*)
    • ignore if the temp folder is cleanup automatically (*)
    • Parse the StdErr and delete the files once the app is closed
  • Use load from memory instead of files (config.DisableTempFileLocking = true;) and disable the cleanup logic (*). This will completely remove debugging and isolation features.
  • Create a new AppDomain and use RazorEngine from there (RazorEngine will handle the cleanup for you)

(*) Disable the RazorEngine Cleanup Logic config.CachingProvider = new DefaultCachingProvider(t => {}); (This will remove the warnings from the StdErr as well)

@niemyjski
Copy link

This is running on azure via web jobs.. Just saying... you can't and
shouldn't assume someone has permissions or access to delete files.
For disabletempfilelocking.. What kind of isolation does it provide?
Any reason why this isn't the default.?

On 6/4/15, matthid notifications@github.com wrote:

Here is a short summary of your options:

  • Do it manually:
    • Clean the temp folder regularly from time to time (*)
    • ignore if the temp folder is cleanup automatically (*)
    • Parse the StdErr and delete the files once the app is closed
  • Use load from memory instead of files (config.DisableTempFileLocking = true;) and disable the cleanup logic (*). This will completely remove
    debugging and isolation features.
  • Create a new AppDomain and use RazorEngine from there (RazorEngine will
    handle the cleanup for you)

(*) Disable the RazorEngine Cleanup Logic config.CachingProvider = new DefaultCachingProvider(t => {}); (This will remove the warnings from the
StdErr as well)


Reply to this email directly or view it on GitHub:
#244 (comment)

Thanks
-Blake Niemyjski

@matthid
Copy link
Collaborator

matthid commented Jun 5, 2015

What kind of isolation does it provide?
Any reason why this isn't the default.?

It removes isolation: Assembly.Load(byte[]) will always load the assembly with full trust. Therefore Razor.Isolation can be circumvented and is therefore without any effect. So it has security implications and is therefore not the default...

@brianharwell
Copy link

Is this going to be fixed? The 3 options are bandaids, not solutions. I upgraded to 3.7.4 from 3.4.1 and I think I am going to revert.

@matthid
Copy link
Collaborator

matthid commented Oct 29, 2015

Reverting is in fact the same as using those options. Sadly there is no easy fix and I don't have much spare time atm.

laedit pushed a commit to Code52/pretzel that referenced this issue Nov 6, 2015
@Greooo
Copy link

Greooo commented Jan 20, 2016

Hello, I upgraded to 3.7.6.0, created a separated AppDomain and temporary dll are not deleted on AppDomain.Unload event. Do diagnose, I implemented the interface RazorEngine.Compilation.CrossAppDomainCleanUp.IPrinter and if I wait 2s. after print the message "cleanup after {0}...", my temporary dll are deleted.

I think there is a bug in the method CrossAppDomainCleanUp.CleanupHelper.DoCleanUp. You remove the entry with '_toCleanup.TryDequeue(out item)' but if the deletion failed, you don't call the 'Enqueue' method for the next try. So, the next try don't find the entry and return true.

If my analysis is correct, is it possible to correct it shortly or have I to let my workaround in my code ?

matthid added a commit that referenced this issue Jan 20, 2016
@matthid
Copy link
Collaborator

matthid commented Jan 20, 2016

@Greooo Nice catch 👍 thanks for the detailed report, fixed in https://www.nuget.org/packages/RazorEngine/3.7.7. Opeing a new issue makes management a bit easier though :) (as this is one already closed)

RoboBurned pushed a commit to RoboBurned/RazorEngine that referenced this issue Aug 25, 2016
@matthid
Copy link
Collaborator

matthid commented Oct 28, 2016

We might have a way to fix this on .netcore after https://github.com/dotnet/coreclr/issues/552

@miksh7
Copy link

miksh7 commented Oct 3, 2017

This fix seems to be a part of 3.10.0 / 4.5.0-rc1 release but I could not find in documentation where/how to configure the path for the temp files.

@AndersBillLinden
Copy link

Why does this package even write to a temporary file? That alone can be a reason for me not using it.

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

Successfully merging a pull request may close this issue.

9 participants