From a498929dd3bf8077ff2e0d7db458490157225256 Mon Sep 17 00:00:00 2001 From: Hao Kung Date: Tue, 22 Sep 2015 21:18:24 -0700 Subject: [PATCH 1/5] Switch to using feature for RequestServices --- .../RequestServicesContainerFeature.cs | 64 +++++++++++++++++++ .../RequestServicesContainerMiddleware.cs | 39 +++++------ 2 files changed, 84 insertions(+), 19 deletions(-) create mode 100644 src/Microsoft.AspNet.Hosting/Internal/RequestServicesContainerFeature.cs diff --git a/src/Microsoft.AspNet.Hosting/Internal/RequestServicesContainerFeature.cs b/src/Microsoft.AspNet.Hosting/Internal/RequestServicesContainerFeature.cs new file mode 100644 index 00000000..bd0b80a2 --- /dev/null +++ b/src/Microsoft.AspNet.Hosting/Internal/RequestServicesContainerFeature.cs @@ -0,0 +1,64 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System; +using Microsoft.AspNet.Http.Features.Internal; +using Microsoft.Framework.DependencyInjection; + +namespace Microsoft.AspNet.Hosting.Internal +{ + public class RequestServicesFeature : IServiceProvidersFeature, IDisposable + { + private readonly IServiceScopeFactory _scopeFactory; + private IServiceProvider _requestServices; + private IServiceScope _scope; + private bool _requestServicesSet; + + public RequestServicesFeature(IServiceProvider applicationServices, IServiceScopeFactory scopeFactory) + { + if (applicationServices == null) + { + throw new ArgumentNullException(nameof(applicationServices)); + } + if (scopeFactory == null) + { + throw new ArgumentNullException(nameof(scopeFactory)); + } + + ApplicationServices = applicationServices; + _scopeFactory = scopeFactory; + } + + public IServiceProvider ApplicationServices { get; set; } + + public IServiceProvider RequestServices + { + get + { + if (!_requestServicesSet) + { + _scope = _scopeFactory.CreateScope(); + _requestServices = _scope.ServiceProvider; + _requestServicesSet = true; + } + return _requestServices; + } + + set + { + _requestServicesSet = true; + RequestServices = value; + } + } + + public void Dispose() + { + if (_scope != null) + { + _scope.Dispose(); + _scope = null; + } + _requestServices = null; + } + } +} \ No newline at end of file diff --git a/src/Microsoft.AspNet.Hosting/Internal/RequestServicesContainerMiddleware.cs b/src/Microsoft.AspNet.Hosting/Internal/RequestServicesContainerMiddleware.cs index ab3fda31..b2e440d9 100644 --- a/src/Microsoft.AspNet.Hosting/Internal/RequestServicesContainerMiddleware.cs +++ b/src/Microsoft.AspNet.Hosting/Internal/RequestServicesContainerMiddleware.cs @@ -5,6 +5,8 @@ using System.Threading.Tasks; using Microsoft.AspNet.Builder; using Microsoft.AspNet.Http; +using Microsoft.AspNet.Http.Features; +using Microsoft.AspNet.Http.Features.Internal; using Microsoft.Framework.DependencyInjection; namespace Microsoft.AspNet.Hosting.Internal @@ -13,20 +15,25 @@ public class RequestServicesContainerMiddleware { private readonly RequestDelegate _next; private readonly IServiceProvider _services; + private readonly IServiceScopeFactory _scopeFactory; - public RequestServicesContainerMiddleware(RequestDelegate next, IServiceProvider services) + public RequestServicesContainerMiddleware(RequestDelegate next, IServiceProvider services, IServiceScopeFactory scopeFactory) { if (next == null) { throw new ArgumentNullException(nameof(next)); } - if (services == null) { throw new ArgumentNullException(nameof(services)); } + if (scopeFactory == null) + { + throw new ArgumentNullException(nameof(scopeFactory)); + } _services = services; + _scopeFactory = scopeFactory; _next = next; } @@ -37,32 +44,26 @@ public async Task Invoke(HttpContext httpContext) throw new ArgumentNullException(nameof(httpContext)); } - // All done if there request services is set - if (httpContext.RequestServices != null) + var existingFeature = httpContext.Features.Get(); + + // All done if request services is set + if (existingFeature?.RequestServices != null) { await _next.Invoke(httpContext); return; } - var priorApplicationServices = httpContext.ApplicationServices; - var serviceProvider = priorApplicationServices ?? _services; - var scopeFactory = serviceProvider.GetRequiredService(); - - try + using (var feature = new RequestServicesFeature(_services, _scopeFactory)) { - // Creates the scope and temporarily swap services - using (var scope = scopeFactory.CreateScope()) + try { - httpContext.ApplicationServices = serviceProvider; - httpContext.RequestServices = scope.ServiceProvider; - + httpContext.Features.Set(feature); await _next.Invoke(httpContext); } - } - finally - { - httpContext.RequestServices = null; - httpContext.ApplicationServices = priorApplicationServices; + finally + { + httpContext.Features.Set(existingFeature); + } } } } From 3b0634a81f821f8d0d5b40634edd64dd50a2638e Mon Sep 17 00:00:00 2001 From: Hao Kung Date: Wed, 23 Sep 2015 12:53:24 -0700 Subject: [PATCH 2/5] Remove explicit ScopeFactory reference --- .../RequestServicesContainerFeature.cs | 10 +--- .../RequestServicesContainerMiddleware.cs | 10 +--- .../TestServerTests.cs | 46 +++++++++++++++++++ 3 files changed, 50 insertions(+), 16 deletions(-) diff --git a/src/Microsoft.AspNet.Hosting/Internal/RequestServicesContainerFeature.cs b/src/Microsoft.AspNet.Hosting/Internal/RequestServicesContainerFeature.cs index bd0b80a2..1bf2d9a0 100644 --- a/src/Microsoft.AspNet.Hosting/Internal/RequestServicesContainerFeature.cs +++ b/src/Microsoft.AspNet.Hosting/Internal/RequestServicesContainerFeature.cs @@ -9,24 +9,18 @@ namespace Microsoft.AspNet.Hosting.Internal { public class RequestServicesFeature : IServiceProvidersFeature, IDisposable { - private readonly IServiceScopeFactory _scopeFactory; private IServiceProvider _requestServices; private IServiceScope _scope; private bool _requestServicesSet; - public RequestServicesFeature(IServiceProvider applicationServices, IServiceScopeFactory scopeFactory) + public RequestServicesFeature(IServiceProvider applicationServices) { if (applicationServices == null) { throw new ArgumentNullException(nameof(applicationServices)); } - if (scopeFactory == null) - { - throw new ArgumentNullException(nameof(scopeFactory)); - } ApplicationServices = applicationServices; - _scopeFactory = scopeFactory; } public IServiceProvider ApplicationServices { get; set; } @@ -37,7 +31,7 @@ public IServiceProvider RequestServices { if (!_requestServicesSet) { - _scope = _scopeFactory.CreateScope(); + _scope = ApplicationServices.GetRequiredService().CreateScope(); _requestServices = _scope.ServiceProvider; _requestServicesSet = true; } diff --git a/src/Microsoft.AspNet.Hosting/Internal/RequestServicesContainerMiddleware.cs b/src/Microsoft.AspNet.Hosting/Internal/RequestServicesContainerMiddleware.cs index b2e440d9..25949db5 100644 --- a/src/Microsoft.AspNet.Hosting/Internal/RequestServicesContainerMiddleware.cs +++ b/src/Microsoft.AspNet.Hosting/Internal/RequestServicesContainerMiddleware.cs @@ -15,9 +15,8 @@ public class RequestServicesContainerMiddleware { private readonly RequestDelegate _next; private readonly IServiceProvider _services; - private readonly IServiceScopeFactory _scopeFactory; - public RequestServicesContainerMiddleware(RequestDelegate next, IServiceProvider services, IServiceScopeFactory scopeFactory) + public RequestServicesContainerMiddleware(RequestDelegate next, IServiceProvider services) { if (next == null) { @@ -27,13 +26,8 @@ public RequestServicesContainerMiddleware(RequestDelegate next, IServiceProvider { throw new ArgumentNullException(nameof(services)); } - if (scopeFactory == null) - { - throw new ArgumentNullException(nameof(scopeFactory)); - } _services = services; - _scopeFactory = scopeFactory; _next = next; } @@ -53,7 +47,7 @@ public async Task Invoke(HttpContext httpContext) return; } - using (var feature = new RequestServicesFeature(_services, _scopeFactory)) + using (var feature = new RequestServicesFeature(_services)) { try { diff --git a/test/Microsoft.AspNet.TestHost.Tests/TestServerTests.cs b/test/Microsoft.AspNet.TestHost.Tests/TestServerTests.cs index ed4bc90a..fac65f77 100644 --- a/test/Microsoft.AspNet.TestHost.Tests/TestServerTests.cs +++ b/test/Microsoft.AspNet.TestHost.Tests/TestServerTests.cs @@ -11,6 +11,8 @@ using Microsoft.AspNet.Hosting; using Microsoft.AspNet.Hosting.Startup; using Microsoft.AspNet.Http; +using Microsoft.AspNet.Http.Features; +using Microsoft.AspNet.Http.Features.Internal; using Microsoft.Framework.Configuration; using Microsoft.Framework.DependencyInjection; using Microsoft.Framework.Logging; @@ -133,6 +135,50 @@ public async Task ExistingRequestServicesWillNotBeReplaced() Assert.Equal("Found:True", result); } + public class ReplaceServiceProvidersFeatureFilter : IStartupFilter, IServiceProvidersFeature + { + public ReplaceServiceProvidersFeatureFilter(IServiceProvider appServices, IServiceProvider requestServices) + { + ApplicationServices = appServices; + RequestServices = requestServices; + } + + public IServiceProvider ApplicationServices { get; set; } + + public IServiceProvider RequestServices { get; set; } + + public Action Configure(Action next) + { + return app => + { + app.Use(async (context, nxt) => + { + context.Features.Set(this); + await nxt(); + }); + next(app); + }; + } + } + + [Fact] + public async Task ExistingServiceProviderFeatureWillNotBeReplaced() + { + var appServices = new ServiceCollection().BuildServiceProvider(); + var server = TestServer.Create(app => + { + app.Run(context => + { + Assert.Equal(appServices, context.ApplicationServices); + Assert.Equal(appServices, context.RequestServices); + return context.Response.WriteAsync("Success"); + }); + }, + services => services.AddInstance(new ReplaceServiceProvidersFeatureFilter(appServices, appServices))); + string result = await server.CreateClient().GetStringAsync("/path"); + Assert.Equal("Success", result); + } + public class EnsureApplicationServicesFilter : IStartupFilter { public Action Configure(Action next) From aeb33f295966930e2336b2fb66d731072f5b528a Mon Sep 17 00:00:00 2001 From: Hao Kung Date: Wed, 23 Sep 2015 13:02:06 -0700 Subject: [PATCH 3/5] Nuke an if --- .../Internal/RequestServicesContainerFeature.cs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/Microsoft.AspNet.Hosting/Internal/RequestServicesContainerFeature.cs b/src/Microsoft.AspNet.Hosting/Internal/RequestServicesContainerFeature.cs index 1bf2d9a0..810635c3 100644 --- a/src/Microsoft.AspNet.Hosting/Internal/RequestServicesContainerFeature.cs +++ b/src/Microsoft.AspNet.Hosting/Internal/RequestServicesContainerFeature.cs @@ -47,11 +47,8 @@ public IServiceProvider RequestServices public void Dispose() { - if (_scope != null) - { - _scope.Dispose(); - _scope = null; - } + _scope?.Dispose(); + _scope = null; _requestServices = null; } } From d01097d52f412975de4e0c883c81ce4a65910cc1 Mon Sep 17 00:00:00 2001 From: Hao Kung Date: Wed, 23 Sep 2015 13:06:32 -0700 Subject: [PATCH 4/5] Add a test --- .../TestServerTests.cs | 39 ++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-) diff --git a/test/Microsoft.AspNet.TestHost.Tests/TestServerTests.cs b/test/Microsoft.AspNet.TestHost.Tests/TestServerTests.cs index fac65f77..ea2406bc 100644 --- a/test/Microsoft.AspNet.TestHost.Tests/TestServerTests.cs +++ b/test/Microsoft.AspNet.TestHost.Tests/TestServerTests.cs @@ -175,7 +175,44 @@ public async Task ExistingServiceProviderFeatureWillNotBeReplaced() }); }, services => services.AddInstance(new ReplaceServiceProvidersFeatureFilter(appServices, appServices))); - string result = await server.CreateClient().GetStringAsync("/path"); + var result = await server.CreateClient().GetStringAsync("/path"); + Assert.Equal("Success", result); + } + + public class NullServiceProvidersFeatureFilter : IStartupFilter, IServiceProvidersFeature + { + public IServiceProvider ApplicationServices { get; set; } + + public IServiceProvider RequestServices { get; set; } + + public Action Configure(Action next) + { + return app => + { + app.Use(async (context, nxt) => + { + context.Features.Set(this); + await nxt(); + }); + next(app); + }; + } + } + + [Fact] + public async Task WillReplaceServiceProviderFeatureWithNullRequestServices() + { + var server = TestServer.Create(app => + { + app.Run(context => + { + Assert.NotNull(context.ApplicationServices); + Assert.NotNull(context.RequestServices); + return context.Response.WriteAsync("Success"); + }); + }, + services => services.AddTransient()); + var result = await server.CreateClient().GetStringAsync("/path"); Assert.Equal("Success", result); } From 35aae08f229f9977d3e2007c1c87145e3851163e Mon Sep 17 00:00:00 2001 From: Hao Kung Date: Wed, 23 Sep 2015 13:28:05 -0700 Subject: [PATCH 5/5] Disallow null ApplicationServices --- .../Internal/RequestServicesContainerFeature.cs | 17 ++++++++++++++++- .../TestServerTests.cs | 16 ++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/src/Microsoft.AspNet.Hosting/Internal/RequestServicesContainerFeature.cs b/src/Microsoft.AspNet.Hosting/Internal/RequestServicesContainerFeature.cs index 810635c3..e3c9779c 100644 --- a/src/Microsoft.AspNet.Hosting/Internal/RequestServicesContainerFeature.cs +++ b/src/Microsoft.AspNet.Hosting/Internal/RequestServicesContainerFeature.cs @@ -9,6 +9,7 @@ namespace Microsoft.AspNet.Hosting.Internal { public class RequestServicesFeature : IServiceProvidersFeature, IDisposable { + private IServiceProvider _appServices; private IServiceProvider _requestServices; private IServiceScope _scope; private bool _requestServicesSet; @@ -23,7 +24,21 @@ public RequestServicesFeature(IServiceProvider applicationServices) ApplicationServices = applicationServices; } - public IServiceProvider ApplicationServices { get; set; } + public IServiceProvider ApplicationServices + { + get + { + return _appServices; + } + set + { + if (value == null) + { + throw new ArgumentNullException(nameof(ApplicationServices)); + } + _appServices = value; + } + } public IServiceProvider RequestServices { diff --git a/test/Microsoft.AspNet.TestHost.Tests/TestServerTests.cs b/test/Microsoft.AspNet.TestHost.Tests/TestServerTests.cs index ea2406bc..a41a9141 100644 --- a/test/Microsoft.AspNet.TestHost.Tests/TestServerTests.cs +++ b/test/Microsoft.AspNet.TestHost.Tests/TestServerTests.cs @@ -135,6 +135,22 @@ public async Task ExistingRequestServicesWillNotBeReplaced() Assert.Equal("Found:True", result); } + [Fact] + public async Task SettingApplicationServicesOnFeatureToNullThrows() + { + var server = TestServer.Create(app => + { + app.Run(context => + { + var feature = context.Features.Get(); + Assert.Throws(() => feature.ApplicationServices = null); + return context.Response.WriteAsync("Success"); + }); + }); + string result = await server.CreateClient().GetStringAsync("/path"); + Assert.Equal("Success", result); + } + public class ReplaceServiceProvidersFeatureFilter : IStartupFilter, IServiceProvidersFeature { public ReplaceServiceProvidersFeatureFilter(IServiceProvider appServices, IServiceProvider requestServices)