Skip to content
This repository has been archived by the owner on Dec 19, 2018. It is now read-only.

Switch to using feature for RequestServices #369

Closed
wants to merge 5 commits into from
Closed

Switch to using feature for RequestServices #369

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a498929
Switch to using feature for RequestServices
HaoK Sep 23, 2015
3b0634a
Remove explicit ScopeFactory reference
HaoK Sep 23, 2015
aeb33f2
Nuke an if
HaoK Sep 23, 2015
d01097d
Add a test
HaoK Sep 23, 2015
35aae08
Disallow null ApplicationServices
HaoK Sep 23, 2015
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
70 changes: 70 additions & 0 deletions src/Microsoft.AspNet.Hosting/Internal/RequestServicesContainerFeature.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
// 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 IServiceProvider _appServices;
private IServiceProvider _requestServices;
private IServiceScope _scope;
private bool _requestServicesSet;

public RequestServicesFeature(IServiceProvider applicationServices)
{
if (applicationServices == null)
{
throw new ArgumentNullException(nameof(applicationServices));
}

ApplicationServices = applicationServices;
}

public IServiceProvider ApplicationServices
{
get
{
return _appServices;
}
set
{
if (value == null)
{
throw new ArgumentNullException(nameof(ApplicationServices));
Copy link
Member

Choose a reason for hiding this comment

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

This should be value not ApplicationServices afaik

}
_appServices = value;
}
}

public IServiceProvider RequestServices
{
get
{
if (!_requestServicesSet)
{
_scope = ApplicationServices.GetRequiredService<IServiceScopeFactory>().CreateScope();
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be defensive since ApplicationServices is settable?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah looks like it. Do we want to throw an exception if you try to set it to null, or just treat that as no-op and basically do nothing and leave RequestServices null if you set ApplicationServices to null?

Copy link
Member

Choose a reason for hiding this comment

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

This behavior is totally undefined. To repro this you would have to set ApplicationServices to null then before ever touching RequestServices (I just know somebody is going to do something stupid and report a bug 😄). This works today because nothing is lazy. We could store the original application services and use that as a fallback but that might be perplexing. Throwing might be the most reasonable thing. Null isn't bad either so lets flip a coin. If you did this as a customer an exception might be the best since you'd want to know why instead of getting a null ref. On the other hand if you get RequestServices and then set ApplicationServices to null, should that throw?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there any real scenario where we want to allow them to set ApplicationServices to null? If not, I can just un auto the property and throw for null sets... Won't we fall over all over the place if ApplicationServices is null? I don't think we are checking anywhere in our code today

Copy link
Member

Choose a reason for hiding this comment

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

I don't know why anybody would set it to null unless they were trying to clear it out. Maybe if the old value was null?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok well this is a subtle breaking change then, setting to null will throw an exception now... updated and added a test

_requestServices = _scope.ServiceProvider;
_requestServicesSet = true;
}
return _requestServices;
}

set
{
_requestServicesSet = true;
RequestServices = value;
}
}

public void Dispose()
{
_scope?.Dispose();
_scope = null;
_requestServices = null;
}
}
}
31 changes: 13 additions & 18 deletions src/Microsoft.AspNet.Hosting/Internal/RequestServicesContainerMiddleware.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -20,7 +22,6 @@ public RequestServicesContainerMiddleware(RequestDelegate next, IServiceProvider
{
throw new ArgumentNullException(nameof(next));
}

if (services == null)
{
throw new ArgumentNullException(nameof(services));
Expand All @@ -37,32 +38,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<IServiceProvidersFeature>();

// 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<IServiceScopeFactory>();

try
using (var feature = new RequestServicesFeature(_services))
{
// Creates the scope and temporarily swap services
using (var scope = scopeFactory.CreateScope())
try
{
httpContext.ApplicationServices = serviceProvider;
httpContext.RequestServices = scope.ServiceProvider;

httpContext.Features.Set<IServiceProvidersFeature>(feature);
await _next.Invoke(httpContext);
}
}
finally
{
httpContext.RequestServices = null;
httpContext.ApplicationServices = priorApplicationServices;
finally
{
httpContext.Features.Set(existingFeature);
}
}
}
}
Expand Down
99 changes: 99 additions & 0 deletions test/Microsoft.AspNet.TestHost.Tests/TestServerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -133,6 +135,103 @@ 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<IServiceProvidersFeature>();
Assert.Throws<ArgumentNullException>(() => 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)
{
ApplicationServices = appServices;
RequestServices = requestServices;
}

public IServiceProvider ApplicationServices { get; set; }

public IServiceProvider RequestServices { get; set; }

public Action<IApplicationBuilder> Configure(Action<IApplicationBuilder> next)
{
return app =>
{
app.Use(async (context, nxt) =>
{
context.Features.Set<IServiceProvidersFeature>(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<IStartupFilter>(new ReplaceServiceProvidersFeatureFilter(appServices, appServices)));
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<IApplicationBuilder> Configure(Action<IApplicationBuilder> next)
{
return app =>
{
app.Use(async (context, nxt) =>
{
context.Features.Set<IServiceProvidersFeature>(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<IStartupFilter, NullServiceProvidersFeatureFilter>());
var result = await server.CreateClient().GetStringAsync("/path");
Assert.Equal("Success", result);
}

public class EnsureApplicationServicesFilter : IStartupFilter
{
public Action<IApplicationBuilder> Configure(Action<IApplicationBuilder> next)
Expand Down