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.

Sign up for GitHub

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

Fixed MVC dependency injection. #3520

Merged
merged 3 commits into from
Jan 31, 2020
Merged

Fixed MVC dependency injection. #3520

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c0b727b
Fixed MVC dependency injection.
dimarobert Jan 24, 2020
72ede51
Add singleton IScopeAccessor service as an abstraction over HttpContext.
dimarobert Jan 31, 2020
3bf179d
Fix unit tests.
dimarobert Jan 31, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 15 additions & 26 deletions DNN Platform/DotNetNuke.Web.Mvc/DnnMvcDependencyResolver.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using System;
using System.Collections.Generic;
using System.Web.Mvc;
using DotNetNuke.Services.DependencyInjection;

namespace DotNetNuke.Web.Mvc
{
Expand All @@ -16,18 +17,10 @@ namespace DotNetNuke.Web.Mvc
internal class DnnMvcDependencyResolver : IDependencyResolver
{
private readonly IServiceProvider _serviceProvider;
private readonly IDependencyResolver _resolver;

/// <summary>
/// Instantiate a new instance of the <see cref="DnnDependencyResolver"/>.
/// </summary>
/// <param name="serviceProvider">
/// The <see cref="IServiceProvider"/> to be used in the <see cref="DnnDependencyResolver"/>
/// </param>
public DnnMvcDependencyResolver(IServiceProvider serviceProvider, IDependencyResolver resolver)
public DnnMvcDependencyResolver(IServiceProvider serviceProvider)
{
_serviceProvider = serviceProvider;
_resolver = resolver;
_serviceProvider = serviceProvider ?? throw new ArgumentNullException(nameof(serviceProvider));
}

/// <summary>
Expand All @@ -41,14 +34,12 @@ public DnnMvcDependencyResolver(IServiceProvider serviceProvider, IDependencyRes
/// </returns>
public object GetService(Type serviceType)
{
try
{
return _serviceProvider.GetService(serviceType);
}
catch
{
return _resolver.GetService(serviceType);
}
var accessor = _serviceProvider.GetRequiredService<IScopeAccessor>();
var scope = accessor.GetScope();
if (scope != null)
return scope.ServiceProvider.GetService(serviceType);
mitchelsellers marked this conversation as resolved.
Show resolved Hide resolved

throw new InvalidOperationException("IServiceScope not provided");
}

/// <summary>
Expand All @@ -62,14 +53,12 @@ public object GetService(Type serviceType)
/// </returns>
public IEnumerable<object> GetServices(Type serviceType)
{
try
{
return _serviceProvider.GetServices(serviceType);
}
catch
{
return _resolver.GetServices(serviceType);
}
var accessor = _serviceProvider.GetRequiredService<IScopeAccessor>();
var scope = accessor.GetScope();
if (scope != null)
return scope.ServiceProvider.GetServices(serviceType);

throw new InvalidOperationException("IServiceScope not provided");
}
}
}
1 change: 1 addition & 0 deletions DNN Platform/DotNetNuke.Web.Mvc/DotNetNuke.Web.Mvc.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@
<Compile Include="Common\PropertyHelper.cs" />
<Compile Include="Common\TypeHelper.cs" />
<Compile Include="DnnMvcDependencyResolver.cs" />
<Compile Include="Extensions\StartupExtensions.cs" />
<Compile Include="Framework\ActionFilters\AuthFilterContext.cs" />
<Compile Include="Framework\ActionFilters\AuthorizeAttributeBase.cs" />
<Compile Include="Framework\ActionFilters\DnnAuthorizeAttribute.cs" />
Expand Down
28 changes: 28 additions & 0 deletions DNN Platform/DotNetNuke.Web.Mvc/Extensions/StartupExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
using DotNetNuke.DependencyInjection.Extensions;
using DotNetNuke.Web.Mvc.Framework.Controllers;
using Microsoft.Extensions.DependencyInjection;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace DotNetNuke.Web.Mvc.Extensions
{
public static class StartupExtensions
{
public static void AddWebApiControllers(this IServiceCollection services)
{
var controllerTypes = AppDomain.CurrentDomain.GetAssemblies()
.SelectMany(TypeExtensions.SafeGetTypes)
.Where(x => typeof(IDnnController).IsAssignableFrom(x)
&& x.IsClass
&& !x.IsAbstract
);
foreach (var controller in controllerTypes)
{
services.AddScoped(controller);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perfect, thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}
}
}
10 changes: 6 additions & 4 deletions DNN Platform/DotNetNuke.Web.Mvc/Startup.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,22 +2,24 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the MIT License. See LICENSE file in the project root for full license information.
//
using DotNetNuke.Common;
using DotNetNuke.DependencyInjection;
using Microsoft.Extensions.DependencyInjection;
using DotNetNuke.Web.Mvc.Extensions;
using System.Web.Mvc;
using DotNetNuke.Common;

namespace DotNetNuke.Web.Mvc
{
public class Startup : IDnnStartup
{
public void ConfigureServices(IServiceCollection services)
{
services.AddSingleton(serviceProvider => ControllerBuilder.Current.GetControllerFactory());
services.AddSingleton<IControllerFactory, DefaultControllerFactory>();
mitchelsellers marked this conversation as resolved.
Show resolved Hide resolved
services.AddSingleton<MvcModuleControlFactory>();

IDependencyResolver resolver = new DnnMvcDependencyResolver(Globals.DependencyProvider, DependencyResolver.Current);
DependencyResolver.SetResolver(resolver);
services.AddWebApiControllers();
mitchelsellers marked this conversation as resolved.
Show resolved Hide resolved

DependencyResolver.SetResolver(new DnnMvcDependencyResolver(Globals.DependencyProvider));
}
}
}
6 changes: 3 additions & 3 deletions DNN Platform/DotNetNuke.Web/Api/Extensions/StartupExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,14 @@ internal static class StartupExtensions
/// </param>
public static void AddWebApi(this IServiceCollection services)
{
var startuptypes = AppDomain.CurrentDomain.GetAssemblies()
var controllerTypes = AppDomain.CurrentDomain.GetAssemblies()
.SelectMany(x => x.SafeGetTypes())
.Where(x => typeof(DnnApiController).IsAssignableFrom(x) &&
x.IsClass &&
!x.IsAbstract);
foreach (var controller in startuptypes)
foreach (var controller in controllerTypes)
{
services.AddTransient(controller);
services.AddScoped(controller);
}
}
}
Expand Down
8 changes: 6 additions & 2 deletions DNN Platform/DotNetNuke.Web/Api/Internal/DnnDependencyResolver.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
using System;
using System.Collections.Generic;
using System.Web.Http.Dependencies;
using DotNetNuke.Common.Extensions;
using DotNetNuke.Services.DependencyInjection;

namespace DotNetNuke.Web.Api.Internal
{
Expand All @@ -25,7 +27,7 @@ internal class DnnDependencyResolver : IDependencyResolver
/// </param>
public DnnDependencyResolver(IServiceProvider serviceProvider)
{
_serviceProvider = serviceProvider;
_serviceProvider = serviceProvider ?? throw new ArgumentNullException(nameof(serviceProvider));
}

/// <summary>
Expand All @@ -36,7 +38,9 @@ public DnnDependencyResolver(IServiceProvider serviceProvider)
/// </returns>
public IDependencyScope BeginScope()
{
return new DnnDependencyResolver(_serviceProvider.CreateScope().ServiceProvider);
var accessor = _serviceProvider.GetRequiredService<IScopeAccessor>();
var scope = accessor.GetScope();
return new DnnDependencyResolver(scope.ServiceProvider);
}

/// <summary>
Expand Down
6 changes: 4 additions & 2 deletions DNN Platform/DotNetNuke.Web/Common/DotNetNukeHttpApplication.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
using DotNetNuke.Instrumentation;
using DotNetNuke.Security.Cookies;
using DotNetNuke.Services.Installer.Blocker;
using Microsoft.Extensions.DependencyInjection;
using DotNetNuke.HttpModules.DependencyInjection;

#endregion

Expand Down Expand Up @@ -67,8 +67,10 @@ private void Application_Start(object sender, EventArgs eventArgs)
var name = Config.GetSetting("ServerName");
Globals.ServerName = String.IsNullOrEmpty(name) ? Dns.GetHostName() : name;

Globals.DependencyProvider = new LazyServiceProvider();
var startup = new Startup();
Globals.DependencyProvider = startup.DependencyProvider;
(Globals.DependencyProvider as LazyServiceProvider).SetProvider(startup.DependencyProvider);
ServiceRequestScopeModule.SetServiceProvider(Globals.DependencyProvider);

ComponentFactory.Container = new SimpleContainer();

Expand Down
26 changes: 26 additions & 0 deletions DNN Platform/DotNetNuke.Web/Common/LazyServiceProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace DotNetNuke.Web.Common
{
public class LazyServiceProvider : IServiceProvider
{
private IServiceProvider _serviceProvider;

public object GetService(Type serviceType)
{
if (_serviceProvider is null)
throw new Exception("Cannot resolve services until the service provider is built.");

return _serviceProvider.GetService(serviceType);
}

internal void SetProvider(IServiceProvider serviceProvider)
{
_serviceProvider = serviceProvider;
}
}
}
1 change: 1 addition & 0 deletions DNN Platform/DotNetNuke.Web/DotNetNuke.Web.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,7 @@
<Compile Include="Common\DynamicSharedConstants.cs" />
<Compile Include="Common\DotNetNukeHttpApplication.cs" />
<Compile Include="Api\WebApiException.cs" />
<Compile Include="Common\LazyServiceProvider.cs" />
<Compile Include="Components\Controllers\ControlBarController.cs" />
<Compile Include="Components\Controllers\IControlBarController.cs" />
<Compile Include="Components\Controllers\Models\MenuItemViewModel.cs" />
Expand Down
2 changes: 2 additions & 0 deletions DNN Platform/DotNetNuke.Web/Startup.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using DotNetNuke.DependencyInjection;
using DotNetNuke.DependencyInjection.Extensions;
using DotNetNuke.Instrumentation;
using DotNetNuke.Services.DependencyInjection;
using DotNetNuke.Web.Extensions;
using Microsoft.Extensions.DependencyInjection;
using System;
Expand All @@ -25,6 +26,7 @@ public Startup()
private void Configure()
{
var services = new ServiceCollection();
services.AddSingleton<IScopeAccessor, ScopeAccessor>();
ConfigureServices(services);
DependencyProvider = services.BuildServiceProvider();
}
Expand Down
65 changes: 65 additions & 0 deletions DNN Platform/HttpModules/DependencyInjection/ServiceRequestScopeModule.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
using System;
using System.Web;
using DotNetNuke.Common.Extensions;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Web.Infrastructure.DynamicModuleHelper;

[assembly: PreApplicationStartMethod(typeof(DotNetNuke.HttpModules.DependencyInjection.ServiceRequestScopeModule), nameof(DotNetNuke.HttpModules.DependencyInjection.ServiceRequestScopeModule.InitModule))]

namespace DotNetNuke.HttpModules.DependencyInjection
{
public class ServiceRequestScopeModule : IHttpModule
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@dimarobert dimarobert Jan 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
public static void InitModule()
{
DynamicModuleUtility.RegisterModule(typeof(ServiceRequestScopeModule));
}

private static IServiceProvider _serviceProvider;

public void Init(HttpApplication context)
{
context.BeginRequest += Context_BeginRequest;
context.EndRequest += Context_EndRequest;
}

public static void SetServiceProvider(IServiceProvider serviceProvider)
{
_serviceProvider = serviceProvider;
}

private void Context_BeginRequest(object sender, EventArgs e)
{
var context = ((HttpApplication)sender).Context;
context.SetScope(_serviceProvider.CreateScope());
}

private void Context_EndRequest(object sender, EventArgs e)
{
var context = ((HttpApplication)sender).Context;
context.GetScope()?.Dispose();
context.ClearScope();
}

/// <summary>
/// Performs application-defined tasks associated with freeing,
/// releasing, or resetting unmanaged resources.
/// </summary>
public void Dispose()
{
Dispose(true);
}

/// <summary>
/// Performs application-defined tasks associated with freeing,
/// releasing, or resetting unmanaged resources.
/// </summary>
/// <param name="disposing">
/// true if the object is currently disposing.
/// </param>
protected virtual void Dispose(bool disposing)
{
// left empty by design
}
}
}
8 changes: 8 additions & 0 deletions DNN Platform/HttpModules/DotNetNuke.HttpModules.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,10 @@
<SpecificVersion>False</SpecificVersion>
<HintPath>..\DotNetNuke.Instrumentation\bin\DotNetNuke.Instrumentation.dll</HintPath>
</Reference>
<Reference Include="Microsoft.Extensions.DependencyInjection.Abstractions, Version=2.1.1.0, Culture=neutral, PublicKeyToken=adb9793829ddae60" />
<Reference Include="Microsoft.Web.Infrastructure, Version=1.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">
<HintPath>..\..\packages\Microsoft.Web.Infrastructure.1.0.0.0\lib\net40\Microsoft.Web.Infrastructure.dll</HintPath>
</Reference>
<Reference Include="System" />
<Reference Include="System.configuration" />
<Reference Include="System.Core" />
Expand All @@ -67,6 +71,7 @@
<Compile Include="..\..\SolutionInfo.cs">
<Link>SolutionInfo.cs</Link>
</Compile>
<Compile Include="DependencyInjection\ServiceRequestScopeModule.cs" />
<Compile Include="MobileRedirect\MobileRedirectModule.cs" />
<Compile Include="OutputCaching\OutputCacheModule.cs" />
<Compile Include="Properties\AssemblyInfo.cs">
Expand Down Expand Up @@ -114,6 +119,9 @@
<ItemGroup>
<Folder Include="UrlRewrite\Config\" />
</ItemGroup>
<ItemGroup>
<None Include="packages.config" />
</ItemGroup>
<Import Project="$(MSBuildBinPath)\Microsoft.CSharp.targets" />
<PropertyGroup>
<PreBuildEvent>
Expand Down
4 changes: 4 additions & 0 deletions DNN Platform/HttpModules/packages.config
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
<?xml version="1.0" encoding="utf-8"?>
<packages>
<package id="Microsoft.Web.Infrastructure" version="1.0.0.0" targetFramework="net472" />
</packages>
41 changes: 41 additions & 0 deletions DNN Platform/Library/Common/Extensions/HttpContextDependencyInjectionExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
using System.Web;
using Microsoft.Extensions.DependencyInjection;

namespace DotNetNuke.Common.Extensions
{
public static class HttpContextDependencyInjectionExtensions
{
public static void SetScope(this HttpContextBase httpContext, IServiceScope scope)
{
httpContext.Items[typeof(IServiceScope)] = scope;
}

public static void SetScope(this HttpContext httpContext, IServiceScope scope)
{
httpContext.Items[typeof(IServiceScope)] = scope;
}

public static void ClearScope(this HttpContext httpContext)
{
httpContext.Items.Remove(typeof(IServiceScope));
}

public static IServiceScope GetScope(this HttpContextBase httpContext)
{
return GetScope(httpContext.Items);
}

public static IServiceScope GetScope(this HttpContext httpContext)
{
return GetScope(httpContext.Items);
}

internal static IServiceScope GetScope(System.Collections.IDictionary contextItems)
{
if (!contextItems.Contains(typeof(IServiceScope)))
return null;

return contextItems[typeof(IServiceScope)] is IServiceScope scope ? scope : null;
}
}
}
Loading