Fixed MVC dependency injection. #3520

dimarobert · 2020-01-24T13:11:21Z

Summary

Added a HttpModule (ServiceRequestScopeModule) that handles DI's request scope lifetime.
- This enables scoped service resolutions for WebForms, MVC and WebApi.
- It also stores the created scope inside the HttpContext.Items for easy access. This can be useful for vendors to access the request scope.
Setup the ServiceRequestScopeModule as part of the Application_Start initialization.
Register the WebApi controllers in the DI container.

Used example from dotnet/aspnetcore#3172 as inspiration.

TODOs:

Properly include the ServiceRequestScopeModule HttpModule through the web.config, get rid of PreApplicationStartMethod assembly attribute in ServiceRequestScopeModule and the Microsoft.Web.Infrastructure package. The module should be the first in the pipeline for BeginRequest and the last for EndRequest, so that as much of the request as possible is covered.
Improve naming and placement for some of the classes created.

* Added a HttpModule (ServiceRequestScopeModule) that handles DI's request scope lifetime. * This enables scoped service resolutions for WebForms, MVC and WebApi. * It also stores the created scope inside the HttpContext.items for easy access (for vendors as well). * Setup the ServiceRequestScopeModule as part of the Application_Start initialization. * Register the WebApi controllers in the DI container.

SkyeHoefling

Thanks so much for contributing this Pull Request, it is great to have other developers working towards the collective goal of better Dependency Injection support in DNN.

The work in this Pull Request looks great but my biggest question, that needs to be answered by @mitchelsellers

Are we comfortable having the Dependency Injection library take a hard dependency on System.Web. This will may create issues when migrating to .NET Core, it may not.
- Since everything is built towards an interface, we should be able to swap it out in theory. I wouldn't be doing my job if I didn't ask this question

Examples:
DnnMvcDependencyResolver
Leverages HttpContext.Current?.GetScope() instead of building our own scope on the fly. This appears to be the correct way to do this in System.Web but now it generates a hard dependency on it

If we are okay taking a hard dependency on System.Web this looks like a great start to maturing the Dependency Injection to follow correct Scope standards in DI implementations

@dimarobert
If I am understanding the code, is the HTTP Module portion needed?

SkyeHoefling · 2020-01-24T13:44:24Z

DNN Platform/DotNetNuke.Web.Mvc/Extensions/StartupExtensions.cs

+                );
+            foreach (var controller in controllerTypes)
+            {
+                services.AddScoped(controller);


Controllers were explicitly removed from the Container because of memory leak problems. In earlier versions of Dependency Injection the controllers that got registered in the container would never be collected by the Garbage Collector. This means if you have 100,000 requests to a specific controller, you would have 100,000 instances of the controller in memory.

I am hesitant to add controllers back in unless there is a very good reason for it

see #3520 (comment) for the reasoning behind the memory leak.

perfect, thanks!

I would like to re-do the same testing that was done to validate the fix that was put in place in 9.4.x to ensure that we don't have a leak, but this looks to be the better way regardless

Please note that if the controllers are not registered in the container it will not be able to resolve them. Which means that in 9.4.4 (apart from the provider being always null) the controllers were always resolved from the asp net DefaultControllerFactory _resolver that was passed in, which only supports parameterless constructors, so no DI for them.

DNN Platform/DotNetNuke.Web.Mvc/DnnMvcDependencyResolver.cs

DNN Platform/DotNetNuke.Web.Mvc/Startup.cs

SkyeHoefling · 2020-01-24T13:51:17Z

DNN Platform/DotNetNuke.Web/Api/Internal/DnnDependencyResolver.cs

@@ -36,7 +37,8 @@ public DnnDependencyResolver(IServiceProvider serviceProvider)
        /// </returns>
        public IDependencyScope BeginScope()
        {
-            return new DnnDependencyResolver(_serviceProvider.CreateScope().ServiceProvider);
+            var scope = System.Web.HttpContext.Current.GetScope();


This actually looks like it is the correct way to handle the DI Scope in System.Web

@ahoefling I assume this is the note of the dependence on System.Web. I'm ok with this as we are using System.Web now, so it makes sense, and this is internal to our stuff

SkyeHoefling · 2020-01-24T13:52:23Z

DNN Platform/HttpModules/DependencyInjection/ServiceRequestScopeModule.cs

+
+namespace DotNetNuke.HttpModules.DependencyInjection
+{
+    public class ServiceRequestScopeModule : IHttpModule


Does this mean you got Dependency Injection working in HTTP Modules or is this needed to access the System.Web Scope?

This HttpModule is the one that handles the entire request scope lifetime. It handles it for all module types (WebForms, MVC, WebApi). For WebForms we need to look at what is injected in the DependecyProvider property of PortalModuleBase. It should be the request scope provider and not the root one.
As stated in the TODOs, this should be the first HttpModule in the pipeline for BeginRequest so that the scope is opened as soon as possible in the request lifetime (all HttpModules after it can access the opened scope through the instance stored in HttpContext.Items - they will not have constructor DI though). The same HttpModule should be the last that executes it's EndRequest so that the scope is disposed as late as possible.

Perfect, thanks for the explanation on what is going on with this code. It helps me a lot and I'm sure it will help others that review this change later.

This sounds like the correct way and I would argue the way I should have implemented this in the first place. I'm actually fine keeping this as an attribute on the class. Something like this shouldn't be configured in the web.config, creates another point of failure on a part of the platform that isn't designed to be plug and play

dimarobert · 2020-01-24T15:50:43Z

Here is an attempt at a complete explanation on how the service resolution works in the dotnet core DI system.

You register the services in one container (IServiceCollection). The services lifetime can be Singleton, Scoped or Transient. You then build a service provider from that container using BuildServiceProvider. This provider is considered the root service provider. It will be created at the startup of the application and disposed when the app is shutdown.

The service provider will hold a cache with all the services that it materialized throughout it's lifetime, no mater their lifetime.
When you resolve a service from the provider, based on the service lifetime, it will:

create the service each time if it is Transient and cache it.
try to get the service from the cache if it's there, otherwise create and cache the service for later resolutions. This applies to both Scoped and Singletons.

When the service provider is disposed it will go through the cache and dispose all services that implement IDisposable.

From the root service provider you can create a scope. What that means is that an IServiceScope instance is created from the root. It will contain a newly created service provider that can access the
root provider's cache, but has it's own cache as well. When you resolve a service from the scope it will:

create the service each time if it is Transient and cache it.
try to get the service from it's cache if it's there, then from the parent (root) provider's cache, otherwise it will create the service and cache it. The caching is a bit different here, if the service lifetime is Scoped, then the scope cache is used, otherwise if it is Singleton, the parent cache is used.

When the scope is disposed all the services that were stored in it's cache and implement IDisposable will be disposed.

Here is a gotcha with this: a Scoped service that is resolved from the root provider will become Singleton. If you think about it, when you resolve a service with Scoped lifetime from the root provider it will create and store it in it's cache, which means that for further resolutions, both the root and any scope providers that are created from it, will always return that cached instance, instead of creating a new one when needed.

Another one would be that Transient services resolved from the root provider will pile up and create a memory leak.

Because of the first one, the Root Service Provider can be created in two ways:

services.BuildServiceProvider(true) - this is considered development mode and scope validations will be done before resolving a service. What it means is that if you try to resolve a Scoped service from the root provider an exception will be thrown so that you know something is wrong and the service changed it's lifetime.
services.BuildServiceProvider() or services.BuildServiceProvider(false) - this is considered production mode, no checks are done, you are on your own.

Conclusion

Request scope is kind of mandatory for a healthy DI architecture.
Also there should be a way to call services.BuildServiceProvider(true) in DNN, for development purposes and for the vendors to easily see if they make a mistake. It should be some kind of switch in web.config.

Disclaimer: I am not completely sure if the Transient services resolved, that are NOT implementing IDisposable, are actually cached by the service provider, but the ones that do are definitely stored so that they are properly disposed.

dimarobert · 2020-01-24T16:35:28Z

I'd rather have someone from the core team understand how everything here works in depth before we go forward.

There are a couple other things that we can try. For example for WebApi we could create a custom IHttpControllerActivator instead of the IDependencyProvider. This will give us access to the HttpRequestMessage and in turn to the HttpContextBase when resolving controllers (decouple from HttpContext.Current). But then, we would not be able to replace services in the asp net pipeline (examples of services) from the container, if that is desired.
Other variant would be to create some sort of singleton service (e.g. IScopeAccessor) that will be updated by the HttpModule on each request with the current scope. Then in the IDependencyProviders we could use the root provider to resolve the IScopeAccessor and resolve services from the current scope.
We can also improve on the IScopeAccessor idea and add some events for scope creation/disposal. This way vendors can hook in and create their own logic. For example we (DnnSharp) have our own container for a while now. To have support in all areas (WebForms, MVC and WebApi) we had to override things in the asp pipeline and use the PreApplicationStartMethod assembly attribute to have a good (not ideal) composition root. Now, looking at a way to be compatible with DNN 9.4.x+, we need to still keep our internal container. That's in part also because we rely on some features that the dotnet core container does not provide, like keyed service registrations and child containers. If we'd have those events we could have a consistent way of managing our internal controller's scope.

mitchelsellers · 2020-01-24T16:38:16Z

@ahoefling @bdukes or others, might we setup a quick conference call

SkyeHoefling · 2020-01-25T16:28:23Z

I'm going to be unavailable on vacation for a week. I'm more than happy to do a more thorough review when I get back the week of 2/3/2020

bogdan-litescu · 2020-01-27T22:58:17Z

To add a bit of business context, DNN Sharp users can't upgrade to DNN 9.4 because of this issue. It would help us if we can tell users an approximate time of release and also if it will be version 9.5 or 9.4.5.

mitchelsellers · 2020-01-28T14:26:01Z

@bogdan-litescu Can you shed any additional light on the impact here? Were you using dependency injection PRIOR to 9.4? This was a new feature in 9.4.x. I just want to make sure that we aren't chasing a red-herring in your case.

dimarobert · 2020-01-28T15:14:00Z

@mitchelsellers yes we are using our own DI for about 3 years now.

bdukes · 2020-01-28T22:01:40Z

@bogdan-litescu @dimarobert so is DNN Sharp planning to update their DI to hook into this request scope?

bogdan-litescu · 2020-01-29T00:35:22Z

@bdukes Robert is implementing a layer of abstraction so on DNN <9.4 we use our previous DI library (DryIoc it's called by the way - very powerful, very lean) and on DNN 9.4+ it will use the one that ships with DNN. With DryIoc we already hook into request scope, MVC, Web API, etc. and actually, that's what's causing the conflicts right now. Our goal is to get similar capabilities with the DNN implementation, otherwise, we'll have to downgrade a lot of code.

By the way, if there's a chance to adopt DryIoc as the DI provider in DNN we would like to have that conversation. The one in .Net core is pretty basic.

bdukes · 2020-01-30T16:58:42Z

I've run a few verification tests, this is looking good to me 👍

I've installed the MVC module from Update for DNN 9.4.4 SkyeHoefling/DNN.DependencyInjection.Samples#3 and verified there's no error with this patch (and there is an error without this patch)
I've installed the test MVC module from Possible memory leak in MVC modules #3344 and the test Web API from Possible memory leak (dependency injection)? #3186 and verified that hitting those URLs repeatedly does not cause an increase in memory usage.

mitchelsellers · 2020-01-30T17:03:13Z

@bdukes I too have tested this in this manner.

There is a TODO: Note from the user about moving the inclusion to the web.config, however, given the nature of how this is done etc, I actually think that the safer route is to leave this implementation as-is and use the attribute.

Thoughts? Do we merge this in since it has been validated and we know that 9.4.4 is broken?

bdukes · 2020-01-30T17:04:33Z

Yes, I agree, leaving it as is would be the best approach, and I'd be good with merging it for 9.5

bdukes · 2020-01-30T17:04:43Z

/azp run

azure-pipelines · 2020-01-30T17:04:49Z

No pipelines are associated with this pull request.

bdukes · 2020-01-30T17:06:36Z

mitchelsellers · 2020-01-30T17:07:13Z

@bdukes Same here....but it appears that a build is in progress......

dimarobert · 2020-01-31T12:45:51Z

@bdukes @mitchelsellers @ahoefling I've made some changes/improvements by adding the IScopeAccessor that I was talking about #3520 (comment). It gets rid of the System.Web dependency in the MVC and WebApi projects.
Take a look dimarobert#1

If that is something that you think is worth it, merge that PR so that the changes reflect here.

bdukes · 2020-01-31T15:22:18Z

Thanks @dimarobert, that change looks good to me. I'm going to build it and re-run my tests, but I think we can go ahead and include that change.

bdukes · 2020-01-31T15:23:54Z

@dimarobert the build is failing with three scope related null reference exceptions. Can you look into those? https://dev.azure.com/dotnet/DNN/_build/results?buildId=27962&view=ms.vss-test-web.build-test-results-tab&runId=1085634&resultId=102042&paneView=debug

bdukes · 2020-01-31T15:24:26Z

Not sure if adding your IScopeAccessor changes will have any effect on those tests, either…

dimarobert · 2020-01-31T15:42:14Z

@bdukes I've made an update to the dimarobert#1 PR to fix the unit tests. The fix was simple due to the IScopeAccessor.

bdukes · 2020-01-31T15:56:01Z

Thanks @dimarobert! I've updated this PR with those changes. They also passed my manual testing (no errors, no memory leaks). I'm good to merge this @mitchelsellers if you are.

david-poindexter · 2020-01-31T16:14:01Z

Awesomeness @dimarobert - great teamwork!

mitchelsellers · 2020-01-31T16:15:03Z

Thanks everyone! This is merged!

SkyeHoefling suggested changes Jan 24, 2020

View reviewed changes

valadas added this to the 9.5.0 milestone Jan 30, 2020

valadas added the Type: Bug label Jan 30, 2020

Add singleton IScopeAccessor service as an abstraction over HttpContext.

72ede51

Fix unit tests.

3bf179d

bdukes approved these changes Jan 31, 2020

View reviewed changes

mitchelsellers approved these changes Jan 31, 2020

View reviewed changes

mitchelsellers changed the title ~~WIP: Fixed MVC dependency injection.~~ Fixed MVC dependency injection. Jan 31, 2020

mitchelsellers marked this pull request as ready for review January 31, 2020 16:12

mitchelsellers merged commit 6231117 into dnnsoftware:develop Jan 31, 2020

dimarobert mentioned this pull request Mar 5, 2020

Support Dependency Injection from Scheduler #3602

Closed

GerardSmit mentioned this pull request Sep 3, 2020

Use request scope in PortalModuleBase #4046

Closed

Fixed MVC dependency injection. #3520

Fixed MVC dependency injection. #3520

Conversation

dimarobert commented Jan 24, 2020 • edited Loading

Summary

SkyeHoefling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimarobert Jan 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimarobert commented Jan 24, 2020

Conclusion

dimarobert commented Jan 24, 2020

mitchelsellers commented Jan 24, 2020

SkyeHoefling commented Jan 25, 2020

bogdan-litescu commented Jan 27, 2020

mitchelsellers commented Jan 28, 2020

dimarobert commented Jan 28, 2020

bdukes commented Jan 28, 2020

bogdan-litescu commented Jan 29, 2020

bdukes commented Jan 30, 2020 • edited Loading

mitchelsellers commented Jan 30, 2020

bdukes commented Jan 30, 2020

bdukes commented Jan 30, 2020

azure-pipelines bot commented Jan 30, 2020

bdukes commented Jan 30, 2020 • edited Loading

mitchelsellers commented Jan 30, 2020

dimarobert commented Jan 31, 2020

bdukes commented Jan 31, 2020

bdukes commented Jan 31, 2020

bdukes commented Jan 31, 2020

dimarobert commented Jan 31, 2020

bdukes commented Jan 31, 2020

david-poindexter commented Jan 31, 2020

mitchelsellers commented Jan 31, 2020

dimarobert commented Jan 24, 2020 •

edited

Loading

dimarobert Jan 24, 2020 •

edited

Loading

bdukes commented Jan 30, 2020 •

edited

Loading

bdukes commented Jan 30, 2020 •

edited

Loading