Skip to content

[SignalR] Move to generic host #22602

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.

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

Merged
merged 9 commits into from
Jun 23, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
<Reference Include="Microsoft.Extensions.DependencyInjection" />
<Reference Include="Microsoft.Extensions.FileProviders.Physical" />
<Reference Include="Microsoft.Extensions.FileProviders.Composite" />
<Reference Include="Microsoft.Extensions.Hosting.Abstractions" />
<Reference Include="Microsoft.Extensions.Hosting" />
<Reference Include="Microsoft.Extensions.Logging" />
<Reference Include="Microsoft.Extensions.Options" />
</ItemGroup>
Expand Down
4 changes: 2 additions & 2 deletions src/Hosting/Hosting/src/Microsoft.AspNetCore.Hosting.csproj
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<Project Sdk="Microsoft.NET.Sdk">
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<Description>ASP.NET Core hosting infrastructure and startup logic for web applications.</Description>
Expand Down Expand Up @@ -26,7 +26,7 @@
<Reference Include="Microsoft.Extensions.DependencyInjection" />
<Reference Include="Microsoft.Extensions.FileProviders.Physical" />
<Reference Include="Microsoft.Extensions.FileProviders.Composite" />
<Reference Include="Microsoft.Extensions.Hosting.Abstractions" />
<Reference Include="Microsoft.Extensions.Hosting" />
<Reference Include="Microsoft.Extensions.Logging" />
<Reference Include="Microsoft.Extensions.Options" />

Expand Down

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ public class HubProtocolVersionTests : FunctionalTestBase
[MemberData(nameof(TransportTypes))]
public async Task ClientUsingOldCallWithOriginalProtocol(HttpTransportType transportType)
{
using (var server = await StartServer<VersionStartup>())
await using (var server = await StartServer<VersionStartup>())
{
var connectionBuilder = new HubConnectionBuilder()
.WithLoggerFactory(LoggerFactory)
Expand Down Expand Up @@ -67,7 +67,7 @@ public async Task ClientUsingOldCallWithOriginalProtocol(HttpTransportType trans
[MemberData(nameof(TransportTypes))]
public async Task ClientUsingOldCallWithNewProtocol(HttpTransportType transportType)
{
using (var server = await StartServer<VersionStartup>())
await using (var server = await StartServer<VersionStartup>())
{
var connectionBuilder = new HubConnectionBuilder()
.WithLoggerFactory(LoggerFactory)
Expand Down Expand Up @@ -100,7 +100,7 @@ public async Task ClientUsingOldCallWithNewProtocol(HttpTransportType transportT
[MemberData(nameof(TransportTypes))]
public async Task ClientUsingNewCallWithNewProtocol(HttpTransportType transportType)
{
using (var server = await StartServer<VersionStartup>())
await using (var server = await StartServer<VersionStartup>())
{
var httpConnectionFactory = new HttpConnectionFactory(
Options.Create(new HttpConnectionOptions
Expand Down Expand Up @@ -166,7 +166,7 @@ bool ExpectedErrors(WriteContext writeContext)
return writeContext.LoggerName == typeof(HubConnection).FullName;
}

using (var server = await StartServer<VersionStartup>(ExpectedErrors))
await using (var server = await StartServer<VersionStartup>(ExpectedErrors))
{
var connectionBuilder = new HubConnectionBuilder()
.WithLoggerFactory(LoggerFactory)
Expand Down
14 changes: 10 additions & 4 deletions src/SignalR/clients/ts/FunctionalTests/Program.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,17 @@
using System.IO;
using System.Runtime.InteropServices;
using System.Security.Cryptography.X509Certificates;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Hosting;
using Microsoft.Extensions.Hosting;
using Microsoft.Extensions.Logging;
using Microsoft.Win32;

namespace FunctionalTests
{
public class Program
{
public static void Main(string[] args)
public static Task Main(string[] args)
{
string url = null;
for (var i = 0; i < args.Length; i++)
Expand All @@ -27,7 +29,10 @@ public static void Main(string[] args)
}
}

var hostBuilder = new WebHostBuilder()
var hostBuilder = new HostBuilder()
.ConfigureWebHost(webHostBuilder =>
{
webHostBuilder
.ConfigureLogging(factory =>
{
factory.AddConsole(options =>
Expand Down Expand Up @@ -82,10 +87,11 @@ public static void Main(string[] args)
if (!string.IsNullOrEmpty(url))
{
Console.WriteLine($"Forcing URL to: {url}");
hostBuilder.UseUrls(url);
webHostBuilder.UseUrls(url);
}
});

hostBuilder.Build().Run();
return hostBuilder.Build().RunAsync();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,13 @@
using Microsoft.AspNetCore.Cors;
using Microsoft.AspNetCore.Cors.Infrastructure;
using Microsoft.AspNetCore.Hosting;
using Microsoft.AspNetCore.Hosting.Server;
using Microsoft.AspNetCore.Hosting.Server.Features;
using Microsoft.AspNetCore.Routing;
using Microsoft.AspNetCore.SignalR.Tests;
using Microsoft.AspNetCore.Testing;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Hosting;
using Microsoft.Extensions.Logging;
using Xunit;
using Xunit.Abstractions;
Expand Down Expand Up @@ -305,7 +307,7 @@ void ConfigureRoutes(IEndpointRouteBuilder endpoints)
[WebSocketsSupportedCondition]
public async Task MapConnectionHandlerWithWebSocketSubProtocolSetsProtocol()
{
var host = BuildWebHost<MyConnectionHandler>("/socket",
using var host = BuildWebHost<MyConnectionHandler>("/socket",
options => options.WebSockets.SubProtocolSelector = subprotocols =>
{
Assert.Equal(new[] { "protocol1", "protocol2" }, subprotocols.ToArray());
Expand All @@ -314,7 +316,7 @@ public async Task MapConnectionHandlerWithWebSocketSubProtocolSetsProtocol()

await host.StartAsync();

var feature = host.ServerFeatures.Get<IServerAddressesFeature>();
var feature = host.Services.GetService<IServer>().Features.Get<IServerAddressesFeature>();
var address = feature.Addresses.First().Replace("http", "ws") + "/socket";

var client = new ClientWebSocket();
Expand Down Expand Up @@ -377,9 +379,12 @@ public override Task OnConnectedAsync(ConnectionContext connection)
}
}

private IWebHost BuildWebHost(Action<IEndpointRouteBuilder> configure)
private IHost BuildWebHost(Action<IEndpointRouteBuilder> configure)
{
return new WebHostBuilder()
return new HostBuilder()
.ConfigureWebHost(webHostBuilder =>
{
webHostBuilder
.UseKestrel()
.ConfigureServices(services =>
{
Expand All @@ -390,13 +395,17 @@ private IWebHost BuildWebHost(Action<IEndpointRouteBuilder> configure)
app.UseRouting();
app.UseEndpoints(endpoints => configure(endpoints));
})
.UseUrls("http://127.0.0.1:0")
.UseUrls("http://127.0.0.1:0");
})
.Build();
}

private IWebHost BuildWebHost<TConnectionHandler>(string path, Action<HttpConnectionDispatcherOptions> configureOptions) where TConnectionHandler : ConnectionHandler
private IHost BuildWebHost<TConnectionHandler>(string path, Action<HttpConnectionDispatcherOptions> configureOptions) where TConnectionHandler : ConnectionHandler
{
return new WebHostBuilder()
return new HostBuilder()
.ConfigureWebHost(webHostBuilder =>
{
webHostBuilder
.UseUrls("http://127.0.0.1:0")
.UseKestrel()
.ConfigureServices(services =>
Expand All @@ -414,6 +423,7 @@ private IWebHost BuildWebHost<TConnectionHandler>(string path, Action<HttpConnec
.ConfigureLogging(factory =>
{
factory.AddXunit(_output, LogLevel.Trace);
});
})
.Build();
}
Expand Down
30 changes: 20 additions & 10 deletions src/SignalR/common/testassets/Tests.Utils/InProcessTestServer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -9,32 +9,32 @@
using System.Threading;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Hosting;
using Microsoft.AspNetCore.Hosting.Server;
using Microsoft.AspNetCore.Hosting.Server.Features;
using Microsoft.AspNetCore.Testing;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Hosting;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Logging.Testing;

namespace Microsoft.AspNetCore.SignalR.Tests
{
public abstract class InProcessTestServer : IDisposable
public abstract class InProcessTestServer : IAsyncDisposable
{
internal abstract event Action<LogRecord> ServerLogged;

public abstract string WebSocketsUrl { get; }

public abstract string Url { get; }

public abstract void Dispose();
public abstract ValueTask DisposeAsync();
}

public class InProcessTestServer<TStartup> : InProcessTestServer
where TStartup : class
{
private readonly ILoggerFactory _loggerFactory;
private readonly ILogger _logger;
private IWebHost _host;
private IHost _host;
private IHostApplicationLifetime _lifetime;
private readonly IDisposable _logToken;
private readonly IDisposable _extraDisposable;
Expand Down Expand Up @@ -91,15 +91,18 @@ private async Task StartServerInner()
// We're using 127.0.0.1 instead of localhost to ensure that we use IPV4 across different OSes
var url = "http://127.0.0.1:0";

_host = new WebHostBuilder()
_host = new HostBuilder()
.ConfigureWebHost(webHostBuilder =>
{
webHostBuilder
.ConfigureLogging(builder => builder
.SetMinimumLevel(LogLevel.Trace)
.AddProvider(new ForwardingLoggerProvider(_loggerFactory)))
.UseStartup(typeof(TStartup))
.UseKestrel()
.UseUrls(url)
.UseContentRoot(Directory.GetCurrentDirectory())
.Build();
.UseContentRoot(Directory.GetCurrentDirectory());
}).Build();

_logger.LogInformation("Starting test server...");
var cts = new CancellationTokenSource(TimeSpan.FromSeconds(30));
Expand All @@ -116,7 +119,7 @@ private async Task StartServerInner()
_logger.LogInformation("Test Server started");

// Get the URL from the server
_url = _host.ServerFeatures.Get<IServerAddressesFeature>().Addresses.Single();
_url = _host.Services.GetService<IServer>().Features.Get<IServerAddressesFeature>().Addresses.Single();

_lifetime = _host.Services.GetRequiredService<IHostApplicationLifetime>();
_lifetime.ApplicationStopped.Register(() =>
Expand Down Expand Up @@ -144,13 +147,20 @@ private string RenderLogs(IList<LogRecord> logs)
return builder.ToString();
}

public override void Dispose()
public override async ValueTask DisposeAsync()
{
try
{
_extraDisposable?.Dispose();
_logger.LogInformation("Shutting down test server");
_logger.LogInformation("Start shutting down test server");
}
finally
{
await _host.StopAsync();
_host.Dispose();
Copy link
Member

Choose a reason for hiding this comment

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

@davidfowl @Tratcher

@BrennanConroy and I learned by investigating test failures related to this change that unlike WebHost.Dispose(), the generic Host.Dispose() just disposes all services.

Even when configured with ConfigureWebHost, Host.Dispose() does not fire ApplicationStopping, ApplicationStopped or call IServer.StopAsync(). Instead KestrelServer just gets disposed which completely bypasses graceful shutdown and aborts all connections. Naturally, this lead to DiagnosticMemoryPool errors in the tests.

I think we should make Host.Dispose() gracefully shutdown the server like WebHost.Dispose() did. Should I file an issue for this?

Copy link
Member

Choose a reason for hiding this comment

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

Feel free to open an issue for triage, it's out of scope for this PR.

I don't think Dispose should be graceful, that's why we added Stop and helpers like Run that call Stop and Dispose for you. I think this was a conscious choice back in 2.2 or 3.0, if not widely discussed.

Copy link
Member

Choose a reason for hiding this comment

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

I was in no way suggesting we change the generic host behavior in this PR. I'm just gauging whether it's worthwhile to even create an issue. It doesn't sound like we're too concerned. Having Dispose() be abortive does make it less sync-over-async which is nice.

_loggerFactory.Dispose();
}
}

private class ForwardingLoggerProvider : ILoggerProvider
{
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<Project Sdk="Microsoft.NET.Sdk">
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<TargetFramework>$(DefaultNetCoreTargetFramework)</TargetFramework>
Expand Down
2 changes: 1 addition & 1 deletion src/SignalR/samples/JwtSample/JwtSample.csproj
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<Project Sdk="Microsoft.NET.Sdk.Web">
<Project Sdk="Microsoft.NET.Sdk.Web">

<PropertyGroup>
<TargetFramework>$(DefaultNetCoreTargetFramework)</TargetFramework>
Expand Down
16 changes: 11 additions & 5 deletions src/SignalR/samples/JwtSample/Program.cs
Original file line number Diff line number Diff line change
@@ -1,17 +1,22 @@
// Copyright (c) .NET Foundation. All rights reserved.
// 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.IO;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Hosting;
using Microsoft.Extensions.Hosting;
using Microsoft.Extensions.Logging;

namespace JwtSample
{
public class Program
{
public static void Main(string[] args)
public static Task Main(string[] args)
{
new WebHostBuilder()
return Host.CreateDefaultBuilder(args)
.ConfigureWebHost(webHostBuilder =>
{
webHostBuilder
.ConfigureLogging(factory =>
{
factory.AddConsole();
Expand All @@ -21,9 +26,10 @@ public static void Main(string[] args)
.UseKestrel()
.UseContentRoot(Directory.GetCurrentDirectory())
.UseIISIntegration()
.UseStartup<Startup>()
.UseStartup<Startup>();
})
.Build()
.Run();
.RunAsync();
}
}
}
18 changes: 12 additions & 6 deletions src/SignalR/samples/SignalRSamples/Program.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,27 +3,33 @@

using System.IO;
using System.Net;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Hosting;
using Microsoft.AspNetCore.SignalR;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.Hosting;
using Microsoft.Extensions.Logging;
using SignalRSamples.Hubs;

namespace SignalRSamples
{
public class Program
{
public static void Main(string[] args)
public static Task Main(string[] args)
{
var config = new ConfigurationBuilder()
.AddCommandLine(args)
.Build();

var host = new WebHostBuilder()
var host = Host.CreateDefaultBuilder(args)
.ConfigureWebHost(webHostBuilder =>
{
webHostBuilder
.UseConfiguration(config)
.UseSetting(WebHostDefaults.PreventHostingStartupKey, "true")
.ConfigureLogging(factory =>
.ConfigureLogging((c, factory) =>
{
factory.AddConfiguration(c.Configuration.GetSection("Logging"));
factory.AddConsole();
})
.UseKestrel(options =>
Expand All @@ -39,10 +45,10 @@ public static void Main(string[] args)
})
.UseContentRoot(Directory.GetCurrentDirectory())
.UseIISIntegration()
.UseStartup<Startup>()
.Build();
.UseStartup<Startup>();
}).Build();

host.Run();
return host.RunAsync();
}
}
}
Loading