-
Notifications
You must be signed in to change notification settings - Fork 308
Refactoring IServerFactory #395 #445
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
// 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; | ||
using Microsoft.AspNet.Http.Features; | ||
|
||
namespace Microsoft.AspNet.Hosting.Server | ||
{ | ||
/// <summary> | ||
/// Represents a server. | ||
/// </summary> | ||
public interface IServer : IDisposable | ||
{ | ||
/// <summary> | ||
/// A collection of HTTP features of the server. | ||
/// </summary> | ||
IFeatureCollection Features { get; } | ||
|
||
/// <summary> | ||
/// Start the server with the given function that processes an HTTP request. | ||
/// </summary> | ||
/// <param name="requestDelegate">A function that processes an HTTP request.</param> | ||
void Start(RequestDelegate requestDelegate); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,16 +1,20 @@ | ||
// 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 System.Threading.Tasks; | ||
using Microsoft.AspNet.Http.Features; | ||
using Microsoft.Extensions.Configuration; | ||
|
||
namespace Microsoft.AspNet.Hosting.Server | ||
{ | ||
/// <summary> | ||
/// Represents a factory for creating servers. | ||
/// </summary> | ||
public interface IServerFactory | ||
{ | ||
IFeatureCollection Initialize(IConfiguration configuration); | ||
IDisposable Start(IFeatureCollection serverFeatures, Func<IFeatureCollection, Task> application); | ||
/// <summary> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here, add some doc comments |
||
/// Creates <see cref="IServer"/> based on the given configuration. | ||
/// </summary> | ||
/// <param name="configuration">An instance of <see cref="IConfiguration"/>.</param> | ||
/// <returns>The created server.</returns> | ||
IServer CreateServer(IConfiguration configuration); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Create or CreateServer? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. According to the issue filed, CreateServer. |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,7 +42,7 @@ public class HostingEngine : IHostingEngine | |
// Only one of these should be set | ||
internal IServerFactory ServerFactory { get; set; } | ||
internal string ServerFactoryLocation { get; set; } | ||
private IFeatureCollection _serverFeatures; | ||
internal IServer Server { get; set; } | ||
|
||
public HostingEngine( | ||
IServiceCollection appServices, | ||
|
@@ -87,15 +87,13 @@ public virtual IApplication Start() | |
var application = BuildApplication(); | ||
|
||
var logger = _applicationServices.GetRequiredService<ILogger<HostingEngine>>(); | ||
var contextFactory = _applicationServices.GetRequiredService<IHttpContextFactory>(); | ||
var diagnosticSource = _applicationServices.GetRequiredService<DiagnosticSource>(); | ||
|
||
logger.Starting(); | ||
|
||
var server = ServerFactory.Start(_serverFeatures, | ||
async features => | ||
Server.Start( | ||
async httpContext => | ||
{ | ||
var httpContext = contextFactory.Create(features); | ||
httpContext.ApplicationServices = _applicationServices; | ||
|
||
if (diagnosticSource.IsEnabled("Microsoft.AspNet.Hosting.BeginRequest")) | ||
|
@@ -135,11 +133,11 @@ public virtual IApplication Start() | |
_applicationLifetime.NotifyStarted(); | ||
logger.Started(); | ||
|
||
return new Application(ApplicationServices, _serverFeatures, new Disposable(() => | ||
return new Application(ApplicationServices, Server.Features, new Disposable(() => | ||
{ | ||
logger.Shutdown(); | ||
_applicationLifetime.StopApplication(); | ||
server.Dispose(); | ||
Server.Dispose(); | ||
_applicationLifetime.NotifyStopped(); | ||
(_applicationServices as IDisposable)?.Dispose(); | ||
})); | ||
|
@@ -191,7 +189,7 @@ private RequestDelegate BuildApplication() | |
EnsureServer(); | ||
|
||
var builderFactory = _applicationServices.GetRequiredService<IApplicationBuilderFactory>(); | ||
var builder = builderFactory.CreateBuilder(_serverFeatures); | ||
var builder = builderFactory.CreateBuilder(Server.Features); | ||
builder.ApplicationServices = _applicationServices; | ||
|
||
var startupFilters = _applicationServices.GetService<IEnumerable<IStartupFilter>>(); | ||
|
@@ -246,21 +244,21 @@ private RequestDelegate BuildApplication() | |
|
||
private void EnsureServer() | ||
{ | ||
if (ServerFactory == null) | ||
if (Server == null) | ||
{ | ||
// Blow up if we don't have a server set at this point | ||
if (ServerFactoryLocation == null) | ||
if (ServerFactory == null) | ||
{ | ||
throw new InvalidOperationException("IHostingBuilder.UseServer() is required for " + nameof(Start) + "()"); | ||
} | ||
// Blow up if we don't have a server set at this point | ||
if (ServerFactoryLocation == null) | ||
{ | ||
throw new InvalidOperationException("IHostingBuilder.UseServer() is required for " + nameof(Start) + "()"); | ||
} | ||
|
||
ServerFactory = _applicationServices.GetRequiredService<IServerLoader>().LoadServerFactory(ServerFactoryLocation); | ||
} | ||
ServerFactory = _applicationServices.GetRequiredService<IServerLoader>().LoadServerFactory(ServerFactoryLocation); | ||
} | ||
|
||
if (_serverFeatures == null) | ||
{ | ||
_serverFeatures = ServerFactory.Initialize(_config); | ||
var addresses = _serverFeatures?.Get<IServerAddressesFeature>()?.Addresses; | ||
Server = ServerFactory.CreateServer(_config); | ||
var addresses = Server.Features?.Get<IServerAddressesFeature>()?.Addresses; | ||
if (addresses != null && !addresses.IsReadOnly) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: you could do if (!addresses?.IsReadOnly) |
||
{ | ||
var port = _config[ServerPort]; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,6 +42,7 @@ public class WebHostBuilder | |
// Only one of these should be set | ||
private string _serverFactoryLocation; | ||
private IServerFactory _serverFactory; | ||
private IServer _server; | ||
|
||
public WebHostBuilder() | ||
: this(config: new ConfigurationBuilder().Build()) | ||
|
@@ -133,6 +134,7 @@ public IHostingEngine Build() | |
var engine = new HostingEngine(hostingServices, startupLoader, _config, _captureStartupErrors); | ||
|
||
// Only one of these should be set, but they are used in priority | ||
engine.Server = _server; | ||
engine.ServerFactory = _serverFactory; | ||
engine.ServerFactoryLocation = _config[ServerKey] ?? _config[OldServerKey] ?? _serverFactoryLocation; | ||
|
||
|
@@ -161,7 +163,18 @@ public WebHostBuilder UseEnvironment(string environment) | |
return this; | ||
} | ||
|
||
public WebHostBuilder UseServer(string assemblyName) | ||
public WebHostBuilder UseServer(IServer server) | ||
{ | ||
if (server == null) | ||
{ | ||
throw new ArgumentNullException(nameof(server)); | ||
} | ||
|
||
_server = server; | ||
return this; | ||
} | ||
|
||
public WebHostBuilder UseServerFactory(string assemblyName) | ||
{ | ||
if (assemblyName == null) | ||
{ | ||
|
@@ -172,8 +185,13 @@ public WebHostBuilder UseServer(string assemblyName) | |
return this; | ||
} | ||
|
||
public WebHostBuilder UseServer(IServerFactory factory) | ||
public WebHostBuilder UseServerFactory(IServerFactory factory) | ||
{ | ||
if (factory == null) | ||
{ | ||
throw new ArgumentNullException(nameof(factory)); | ||
} | ||
|
||
_serverFactory = factory; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a null check here for consistency? |
||
return this; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,21 +23,27 @@ namespace Microsoft.AspNet.TestHost | |
/// </summary> | ||
public class ClientHandler : HttpMessageHandler | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This type shouldn't have changed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's wrong here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should be using the default http context factory that sets the accessor for us automatically. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The default http context factory is less convienet to use here, but it would make it so you don't have to think about the accessor. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yah, so use it |
||
{ | ||
private readonly Func<IFeatureCollection, Task> _next; | ||
private readonly RequestDelegate _next; | ||
private readonly PathString _pathBase; | ||
private readonly IHttpContextFactory _factory; | ||
|
||
/// <summary> | ||
/// Create a new handler. | ||
/// </summary> | ||
/// <param name="next">The pipeline entry point.</param> | ||
public ClientHandler(Func<IFeatureCollection, Task> next, PathString pathBase) | ||
public ClientHandler(RequestDelegate next, PathString pathBase, IHttpContextFactory httpContextFactory) | ||
{ | ||
if (next == null) | ||
{ | ||
throw new ArgumentNullException(nameof(next)); | ||
} | ||
if (httpContextFactory == null) | ||
{ | ||
throw new ArgumentNullException(nameof(httpContextFactory)); | ||
} | ||
|
||
_next = next; | ||
_factory = httpContextFactory; | ||
|
||
// PathString.StartsWithSegments that we use below requires the base path to not end in a slash. | ||
if (pathBase.HasValue && pathBase.Value.EndsWith("/")) | ||
|
@@ -63,7 +69,7 @@ protected override async Task<HttpResponseMessage> SendAsync( | |
throw new ArgumentNullException(nameof(request)); | ||
} | ||
|
||
var state = new RequestState(request, _pathBase); | ||
var state = new RequestState(request, _pathBase, _factory); | ||
var requestContent = request.Content ?? new StreamContent(Stream.Null); | ||
var body = await requestContent.ReadAsStreamAsync(); | ||
if (body.CanSeek) | ||
|
@@ -79,7 +85,7 @@ protected override async Task<HttpResponseMessage> SendAsync( | |
{ | ||
try | ||
{ | ||
await _next(state.HttpContext.Features); | ||
await _next(state.HttpContext); | ||
state.CompleteResponse(); | ||
} | ||
catch (Exception ex) | ||
|
@@ -88,6 +94,7 @@ protected override async Task<HttpResponseMessage> SendAsync( | |
} | ||
finally | ||
{ | ||
state.ServerCleanup(); | ||
registration.Dispose(); | ||
} | ||
}); | ||
|
@@ -102,14 +109,16 @@ private class RequestState | |
private ResponseStream _responseStream; | ||
private ResponseFeature _responseFeature; | ||
private CancellationTokenSource _requestAbortedSource; | ||
private IHttpContextFactory _factory; | ||
private bool _pipelineFinished; | ||
|
||
internal RequestState(HttpRequestMessage request, PathString pathBase) | ||
internal RequestState(HttpRequestMessage request, PathString pathBase, IHttpContextFactory factory) | ||
{ | ||
_request = request; | ||
_responseTcs = new TaskCompletionSource<HttpResponseMessage>(); | ||
_requestAbortedSource = new CancellationTokenSource(); | ||
_pipelineFinished = false; | ||
_factory = factory; | ||
|
||
if (request.RequestUri.IsDefaultPort) | ||
{ | ||
|
@@ -120,7 +129,8 @@ internal RequestState(HttpRequestMessage request, PathString pathBase) | |
request.Headers.Host = request.RequestUri.GetComponents(UriComponents.HostAndPort, UriFormat.UriEscaped); | ||
} | ||
|
||
HttpContext = new DefaultHttpContext(); | ||
HttpContext = _factory.Create(new FeatureCollection()); | ||
|
||
HttpContext.Features.Set<IHttpRequestFeature>(new RequestFeature()); | ||
_responseFeature = new ResponseFeature(); | ||
HttpContext.Features.Set<IHttpResponseFeature>(_responseFeature); | ||
|
@@ -228,6 +238,14 @@ internal void Abort(Exception exception) | |
_responseStream.Abort(exception); | ||
_responseTcs.TrySetException(exception); | ||
} | ||
|
||
internal void ServerCleanup() | ||
{ | ||
if (HttpContext != null) | ||
{ | ||
_factory.Dispose(HttpContext); | ||
} | ||
} | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add some doc comments