toddams / RazorLight

Template engine based on Microsoft's Razor parsing engine for .NET Core
Apache License 2.0
1.51k stars 261 forks source link

PropertyInjector is not scoping properly #372

Open ldubrois opened 3 years ago

ldubrois commented 3 years ago

Describe the bug We are using RazorLight in a project, with PropertyInjector as a PreRenderCallback. We are injecting a service in a Razor view (using the form @inject) ; this service requires a IServiceProvider. When using the IServiceProvider in the class, we a have a Object Disposed exception.

To Reproduce Use injection with a class requiring an IServiceProvider and try to invoke it.

Expected behavior The exception should not occurs.

Information (please complete the following information):

Additional context It seems from the documentation provided by Microsoft, that a IServiceProvider lifetime is "scope", and this is why we are getting an exception.

Replacing the PropertyInjector by a very simple code solve the problem (by removing the scopes)

            engine.Options.PreRenderCallbacks.Add(template =>
            {
                PropertyInfo[] properties = template.GetType().GetRuntimeProperties()
                                                .Where(p =>
                                                {
                                                    return
                                                    p.IsDefined(typeof(RazorInjectAttribute))
                                                    &&
                                                    p.GetIndexParameters().Length ==
                                                    0 &&
                                                    !p.SetMethod.IsStatic;
                                                }).ToArray();

                foreach (var property in properties)
                {
                    Type memberType = property.PropertyType;
                    object instance = ServiceProvider.GetRequiredService(memberType);

                    property.SetValue(template, instance);
                }

Thanks,

jzabroski commented 3 years ago

@ldubrois Are you looking to submit a PR? It seems like you did most of the work already? Assuming the existing tests passed.

There are a couple of things not ideal with the way we're using IServiceProvider. The big thing is the RazorLightEngineBuilder isn't composable via "services".

ldubrois commented 3 years ago

I'm not used to github to create PR but I can try 😀

The only think that concerns me is the possible side effect of removing the scopes. May be some people take advantage of it (but don't really see how because the scope is completely dissociated from the render of the view itself).

From what I've seen, in classic MVC website, the scope is for the entire request, so if we want to reproduce it, the scope should be created on the pre-render and disposed after the render.

I don't know well RazorLight, but is there something similar to ASP.NET pipelines (collection of middleware) in which we could create the scope and dispose it ?

jzabroski commented 3 years ago

@ldubrois The tutorial is here: https://docs.github.com/en/github/collaborating-with-issues-and-pull-requests/creating-a-pull-request

There are two reasons IServiceProvider might cause problems:

  1. Calls to BuildServiceProvider - in ASP.NET Core, these are supposed to be called by the framework. In ASP.NET MVC Classic, I am less certain on the rules and whether IServiceProvider is actually part of the old pipeline. I think with the old pipeline, the scope is really whatever you choose it to be. In my programs, I believe I do it at begin request and end request, as that guarantees even if there is an exception I dispose of the resource. See: https://www.strathweb.com/2015/08/disposing-resources-at-the-end-of-web-api-request/
  2. If you call BuildServiceProvider twice. Note that RazorLightEngineBuilder does not call BuildServiceProvider, so if you're using that interface, then you should be safe. However, some people have followed code online that does not use RazorLightEngineBuilder, and they are confused why some stuff doesn't work.
jzabroski commented 3 years ago

@ldubrois Can you answer whether you were using RazorLightEngineBuilder or not? Thanks. It would help save time writing a regression test to cover this scenario.

jzabroski commented 3 years ago

@ldubrois Any update?

Place1 commented 3 years ago

I believe i've run into the same/related problem.

I'm currently using 2.0.0-rc2 and I believe this code is problematic.

The linked code from the PropertyInjector creates a scope and sets the @inject fields inside the template but then disposes the service scope which will dispose any IDisposable services it created.

In my case I'm attempting to use asp.net's UserManager which is an IDisposable. The UserManager is correctly injected but because it's disposed before the template is actually rendered it'll throw an error:

Object name: 'UserManager`1'.
  Stack Trace:
     at Microsoft.AspNetCore.Identity.UserManager`1.ThrowIfDisposed()