From 8b1a7e9199583e181bb0cbdb38481f6259dbd0c1 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Thu, 12 Apr 2018 03:50:14 -0700 Subject: [PATCH] API review feedback (#1974) - Remove SetHttpContext - Remove HttpContextFeature - Add and implement IHttpTransportFeature on HttpConnectionContext - Remove ConnectionMetadataNames --- clients/ts/FunctionalTests/TestHub.cs | 3 +- .../MessagesConnectionHandler.cs | 7 +++-- .../ConnectionMetadataNames.cs | 10 ------- .../Features/IHttpContextFeature.cs | 5 ---- .../Features/IHttpTransportFeature.cs | 15 ++++++++++ .../HttpConnectionContext.cs | 4 ++- .../HttpConnectionContextExtensions.cs | 11 ------- .../HttpConnectionDispatcher.cs | 1 - .../HttpConnectionDispatcherTests.cs | 30 +------------------ .../Hubs.cs | 3 +- .../HubConnectionHandlerTests.cs | 12 +++++++- 11 files changed, 39 insertions(+), 62 deletions(-) delete mode 100644 src/Microsoft.AspNetCore.Http.Connections/ConnectionMetadataNames.cs create mode 100644 src/Microsoft.AspNetCore.Http.Connections/Features/IHttpTransportFeature.cs diff --git a/clients/ts/FunctionalTests/TestHub.cs b/clients/ts/FunctionalTests/TestHub.cs index c996916277..6c4275ee91 100644 --- a/clients/ts/FunctionalTests/TestHub.cs +++ b/clients/ts/FunctionalTests/TestHub.cs @@ -6,6 +6,7 @@ using System.Reactive.Linq; using System.Threading.Channels; using System.Threading.Tasks; using Microsoft.AspNetCore.Http.Connections; +using Microsoft.AspNetCore.Http.Connections.Features; using Microsoft.AspNetCore.SignalR; namespace FunctionalTests @@ -63,7 +64,7 @@ namespace FunctionalTests public string GetActiveTransportName() { - return Context.Items[ConnectionMetadataNames.Transport].ToString(); + return Context.Features.Get().TransportType.ToString(); } public ComplexObject EchoComplexObject(ComplexObject complexObject) diff --git a/samples/SignalRSamples/ConnectionHandlers/MessagesConnectionHandler.cs b/samples/SignalRSamples/ConnectionHandlers/MessagesConnectionHandler.cs index 4df01bfd5c..46be30d6e3 100644 --- a/samples/SignalRSamples/ConnectionHandlers/MessagesConnectionHandler.cs +++ b/samples/SignalRSamples/ConnectionHandlers/MessagesConnectionHandler.cs @@ -7,6 +7,7 @@ using System.Text; using System.Threading.Tasks; using Microsoft.AspNetCore.Connections; using Microsoft.AspNetCore.Http.Connections; +using Microsoft.AspNetCore.Http.Connections.Features; namespace SignalRSamples.ConnectionHandlers { @@ -18,7 +19,9 @@ namespace SignalRSamples.ConnectionHandlers { Connections.Add(connection); - await Broadcast($"{connection.ConnectionId} connected ({connection.Items[ConnectionMetadataNames.Transport]})"); + var transportType = connection.Features.Get()?.TransportType; + + await Broadcast($"{connection.ConnectionId} connected ({transportType})"); try { @@ -51,7 +54,7 @@ namespace SignalRSamples.ConnectionHandlers { Connections.Remove(connection); - await Broadcast($"{connection.ConnectionId} disconnected ({connection.Items[ConnectionMetadataNames.Transport]})"); + await Broadcast($"{connection.ConnectionId} disconnected ({transportType})"); } } diff --git a/src/Microsoft.AspNetCore.Http.Connections/ConnectionMetadataNames.cs b/src/Microsoft.AspNetCore.Http.Connections/ConnectionMetadataNames.cs deleted file mode 100644 index 33aa7a0529..0000000000 --- a/src/Microsoft.AspNetCore.Http.Connections/ConnectionMetadataNames.cs +++ /dev/null @@ -1,10 +0,0 @@ -// 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. - -namespace Microsoft.AspNetCore.Http.Connections -{ - public static class ConnectionMetadataNames - { - public static readonly string Transport = nameof(Transport); - } -} diff --git a/src/Microsoft.AspNetCore.Http.Connections/Features/IHttpContextFeature.cs b/src/Microsoft.AspNetCore.Http.Connections/Features/IHttpContextFeature.cs index 5650f63056..70c4d9e854 100644 --- a/src/Microsoft.AspNetCore.Http.Connections/Features/IHttpContextFeature.cs +++ b/src/Microsoft.AspNetCore.Http.Connections/Features/IHttpContextFeature.cs @@ -12,9 +12,4 @@ namespace Microsoft.AspNetCore.Http.Connections.Features { HttpContext HttpContext { get; set; } } - - public class HttpContextFeature : IHttpContextFeature - { - public HttpContext HttpContext { get; set; } - } } diff --git a/src/Microsoft.AspNetCore.Http.Connections/Features/IHttpTransportFeature.cs b/src/Microsoft.AspNetCore.Http.Connections/Features/IHttpTransportFeature.cs new file mode 100644 index 0000000000..47354f1013 --- /dev/null +++ b/src/Microsoft.AspNetCore.Http.Connections/Features/IHttpTransportFeature.cs @@ -0,0 +1,15 @@ +// 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.Collections.Generic; +using System.Text; +using Microsoft.AspNetCore.Http; + +namespace Microsoft.AspNetCore.Http.Connections.Features +{ + public interface IHttpTransportFeature + { + HttpTransportType TransportType { get; } + } +} diff --git a/src/Microsoft.AspNetCore.Http.Connections/HttpConnectionContext.cs b/src/Microsoft.AspNetCore.Http.Connections/HttpConnectionContext.cs index dd70caddf4..92a80ba310 100644 --- a/src/Microsoft.AspNetCore.Http.Connections/HttpConnectionContext.cs +++ b/src/Microsoft.AspNetCore.Http.Connections/HttpConnectionContext.cs @@ -24,7 +24,8 @@ namespace Microsoft.AspNetCore.Http.Connections IConnectionUserFeature, IConnectionHeartbeatFeature, ITransferFormatFeature, - IHttpContextFeature + IHttpContextFeature, + IHttpTransportFeature { private readonly object _heartbeatLock = new object(); private List<(Action handler, object state)> _heartbeatHandlers; @@ -62,6 +63,7 @@ namespace Microsoft.AspNetCore.Http.Connections Features.Set(this); Features.Set(this); Features.Set(this); + Features.Set(this); } public HttpConnectionContext(string id, IDuplexPipe transport, IDuplexPipe application, ILogger logger = null) diff --git a/src/Microsoft.AspNetCore.Http.Connections/HttpConnectionContextExtensions.cs b/src/Microsoft.AspNetCore.Http.Connections/HttpConnectionContextExtensions.cs index 79fb03f5b5..48364f8ad9 100644 --- a/src/Microsoft.AspNetCore.Http.Connections/HttpConnectionContextExtensions.cs +++ b/src/Microsoft.AspNetCore.Http.Connections/HttpConnectionContextExtensions.cs @@ -13,16 +13,5 @@ namespace Microsoft.AspNetCore.Http.Connections { return connection.Features.Get()?.HttpContext; } - - public static void SetHttpContext(this ConnectionContext connection, HttpContext httpContext) - { - var feature = connection.Features.Get(); - if (feature == null) - { - feature = new HttpContextFeature(); - connection.Features.Set(feature); - } - feature.HttpContext = httpContext; - } } } diff --git a/src/Microsoft.AspNetCore.Http.Connections/HttpConnectionDispatcher.cs b/src/Microsoft.AspNetCore.Http.Connections/HttpConnectionDispatcher.cs index d7bb2b6348..35e2152706 100644 --- a/src/Microsoft.AspNetCore.Http.Connections/HttpConnectionDispatcher.cs +++ b/src/Microsoft.AspNetCore.Http.Connections/HttpConnectionDispatcher.cs @@ -504,7 +504,6 @@ namespace Microsoft.AspNetCore.Http.Connections if (connection.TransportType == HttpTransportType.None) { connection.TransportType = transportType; - connection.Items[ConnectionMetadataNames.Transport] = transportType; } else if (connection.TransportType != transportType) { diff --git a/test/Microsoft.AspNetCore.Http.Connections.Tests/HttpConnectionDispatcherTests.cs b/test/Microsoft.AspNetCore.Http.Connections.Tests/HttpConnectionDispatcherTests.cs index 769d9505f6..ed06e7c19c 100644 --- a/test/Microsoft.AspNetCore.Http.Connections.Tests/HttpConnectionDispatcherTests.cs +++ b/test/Microsoft.AspNetCore.Http.Connections.Tests/HttpConnectionDispatcherTests.cs @@ -107,7 +107,6 @@ namespace Microsoft.AspNetCore.Http.Connections.Tests var pipeOptions = new PipeOptions(pauseWriterThreshold: 8, resumeWriterThreshold: 4); var connection = manager.CreateConnection(pipeOptions, pipeOptions); connection.TransportType = transportType; - connection.Items[ConnectionMetadataNames.Transport] = transportType; using (var requestBody = new MemoryStream()) using (var responseBody = new MemoryStream()) @@ -265,7 +264,6 @@ namespace Microsoft.AspNetCore.Http.Connections.Tests var dispatcher = new HttpConnectionDispatcher(manager, loggerFactory); var connection = manager.CreateConnection(); connection.TransportType = HttpTransportType.WebSockets; - connection.Items[ConnectionMetadataNames.Transport] = HttpTransportType.WebSockets; using (var strm = new MemoryStream()) { @@ -303,7 +301,6 @@ namespace Microsoft.AspNetCore.Http.Connections.Tests var dispatcher = new HttpConnectionDispatcher(manager, loggerFactory); var connection = manager.CreateConnection(); connection.TransportType = HttpTransportType.LongPolling; - connection.Items[ConnectionMetadataNames.Transport] = HttpTransportType.LongPolling; await connection.DisposeAsync(closeGracefully: false); using (var strm = new MemoryStream()) @@ -342,7 +339,6 @@ namespace Microsoft.AspNetCore.Http.Connections.Tests var dispatcher = new HttpConnectionDispatcher(manager, loggerFactory); var connection = manager.CreateConnection(); connection.TransportType = transportType; - connection.Items[ConnectionMetadataNames.Transport] = transportType; using (var strm = new MemoryStream()) { @@ -404,7 +400,6 @@ namespace Microsoft.AspNetCore.Http.Connections.Tests var dispatcher = new HttpConnectionDispatcher(manager, loggerFactory); var connection = manager.CreateConnection(); connection.TransportType = HttpTransportType.LongPolling; - connection.Items[ConnectionMetadataNames.Transport] = HttpTransportType.LongPolling; using (var strm = new MemoryStream()) { @@ -468,7 +463,6 @@ namespace Microsoft.AspNetCore.Http.Connections.Tests var dispatcher = new HttpConnectionDispatcher(manager, loggerFactory); var connection = manager.CreateConnection(); connection.TransportType = transportType; - connection.Items[ConnectionMetadataNames.Transport] = transportType; using (var requestBody = new MemoryStream()) using (var responseBody = new MemoryStream()) @@ -518,7 +512,6 @@ namespace Microsoft.AspNetCore.Http.Connections.Tests var dispatcher = new HttpConnectionDispatcher(manager, loggerFactory); var connection = manager.CreateConnection(); connection.TransportType = transportType; - connection.Items[ConnectionMetadataNames.Transport] = transportType; // Allow a maximum of one caller to use code at one time var callerTracker = new SemaphoreSlim(1, 1); @@ -604,7 +597,6 @@ namespace Microsoft.AspNetCore.Http.Connections.Tests var dispatcher = new HttpConnectionDispatcher(manager, loggerFactory); var connection = manager.CreateConnection(); connection.TransportType = HttpTransportType.LongPolling; - connection.Items[ConnectionMetadataNames.Transport] = HttpTransportType.LongPolling; using (var requestBody = new MemoryStream()) using (var responseBody = new MemoryStream()) @@ -804,7 +796,6 @@ namespace Microsoft.AspNetCore.Http.Connections.Tests var manager = CreateConnectionManager(loggerFactory); var connection = manager.CreateConnection(); connection.TransportType = HttpTransportType.ServerSentEvents; - connection.Items[ConnectionMetadataNames.Transport] = HttpTransportType.ServerSentEvents; var dispatcher = new HttpConnectionDispatcher(manager, loggerFactory); @@ -833,7 +824,6 @@ namespace Microsoft.AspNetCore.Http.Connections.Tests var manager = CreateConnectionManager(loggerFactory); var connection = manager.CreateConnection(); connection.TransportType = HttpTransportType.ServerSentEvents; - connection.Items[ConnectionMetadataNames.Transport] = HttpTransportType.ServerSentEvents; var dispatcher = new HttpConnectionDispatcher(manager, loggerFactory); var context = MakeRequest("/foo", connection); @@ -861,7 +851,6 @@ namespace Microsoft.AspNetCore.Http.Connections.Tests var manager = CreateConnectionManager(loggerFactory); var connection = manager.CreateConnection(); connection.TransportType = HttpTransportType.LongPolling; - connection.Items[ConnectionMetadataNames.Transport] = HttpTransportType.LongPolling; var dispatcher = new HttpConnectionDispatcher(manager, loggerFactory); @@ -889,7 +878,6 @@ namespace Microsoft.AspNetCore.Http.Connections.Tests var manager = CreateConnectionManager(loggerFactory); var connection = manager.CreateConnection(); connection.TransportType = HttpTransportType.LongPolling; - connection.Items[ConnectionMetadataNames.Transport] = HttpTransportType.LongPolling; var dispatcher = new HttpConnectionDispatcher(manager, loggerFactory); @@ -916,7 +904,6 @@ namespace Microsoft.AspNetCore.Http.Connections.Tests var manager = CreateConnectionManager(loggerFactory); var connection = manager.CreateConnection(); connection.TransportType = HttpTransportType.WebSockets; - connection.Items[ConnectionMetadataNames.Transport] = HttpTransportType.WebSockets; var dispatcher = new HttpConnectionDispatcher(manager, loggerFactory); @@ -947,7 +934,6 @@ namespace Microsoft.AspNetCore.Http.Connections.Tests var manager = CreateConnectionManager(loggerFactory); var connection = manager.CreateConnection(); connection.TransportType = transportType; - connection.Items[ConnectionMetadataNames.Transport] = transportType; var dispatcher = new HttpConnectionDispatcher(manager, loggerFactory); @@ -991,7 +977,6 @@ namespace Microsoft.AspNetCore.Http.Connections.Tests var manager = CreateConnectionManager(loggerFactory); var connection = manager.CreateConnection(); connection.TransportType = HttpTransportType.LongPolling; - connection.Items[ConnectionMetadataNames.Transport] = HttpTransportType.LongPolling; var dispatcher = new HttpConnectionDispatcher(manager, loggerFactory); @@ -1030,7 +1015,6 @@ namespace Microsoft.AspNetCore.Http.Connections.Tests var manager = CreateConnectionManager(loggerFactory); var connection = manager.CreateConnection(); connection.TransportType = transportType; - connection.Items[ConnectionMetadataNames.Transport] = transportType; connection.Status = HttpConnectionContext.ConnectionStatus.Disposed; var dispatcher = new HttpConnectionDispatcher(manager, loggerFactory); @@ -1059,7 +1043,6 @@ namespace Microsoft.AspNetCore.Http.Connections.Tests var manager = CreateConnectionManager(loggerFactory); var connection = manager.CreateConnection(); connection.TransportType = HttpTransportType.LongPolling; - connection.Items[ConnectionMetadataNames.Transport] = HttpTransportType.LongPolling; var dispatcher = new HttpConnectionDispatcher(manager, loggerFactory); @@ -1095,7 +1078,6 @@ namespace Microsoft.AspNetCore.Http.Connections.Tests var manager = CreateConnectionManager(loggerFactory); var connection = manager.CreateConnection(); connection.TransportType = HttpTransportType.ServerSentEvents; - connection.Items[ConnectionMetadataNames.Transport] = HttpTransportType.ServerSentEvents; var dispatcher = new HttpConnectionDispatcher(manager, loggerFactory); @@ -1131,7 +1113,6 @@ namespace Microsoft.AspNetCore.Http.Connections.Tests var manager = CreateConnectionManager(loggerFactory); var connection = manager.CreateConnection(); connection.TransportType = HttpTransportType.LongPolling; - connection.Items[ConnectionMetadataNames.Transport] = HttpTransportType.LongPolling; var dispatcher = new HttpConnectionDispatcher(manager, loggerFactory); @@ -1166,7 +1147,6 @@ namespace Microsoft.AspNetCore.Http.Connections.Tests var manager = CreateConnectionManager(loggerFactory); var connection = manager.CreateConnection(); connection.TransportType = HttpTransportType.LongPolling; - connection.Items[ConnectionMetadataNames.Transport] = HttpTransportType.LongPolling; var dispatcher = new HttpConnectionDispatcher(manager, loggerFactory); @@ -1209,7 +1189,6 @@ namespace Microsoft.AspNetCore.Http.Connections.Tests var manager = CreateConnectionManager(loggerFactory); var connection = manager.CreateConnection(); connection.TransportType = transportType; - connection.Items[ConnectionMetadataNames.Transport] = transportType; var dispatcher = new HttpConnectionDispatcher(manager, loggerFactory); @@ -1242,7 +1221,6 @@ namespace Microsoft.AspNetCore.Http.Connections.Tests var manager = CreateConnectionManager(loggerFactory); var connection = manager.CreateConnection(); connection.TransportType = HttpTransportType.LongPolling; - connection.Items[ConnectionMetadataNames.Transport] = HttpTransportType.LongPolling; var dispatcher = new HttpConnectionDispatcher(manager, loggerFactory); var context = new DefaultHttpContext(); var services = new ServiceCollection(); @@ -1289,7 +1267,6 @@ namespace Microsoft.AspNetCore.Http.Connections.Tests var manager = CreateConnectionManager(loggerFactory); var connection = manager.CreateConnection(); connection.TransportType = HttpTransportType.LongPolling; - connection.Items[ConnectionMetadataNames.Transport] = HttpTransportType.LongPolling; var dispatcher = new HttpConnectionDispatcher(manager, loggerFactory); var context = new DefaultHttpContext(); var services = new ServiceCollection(); @@ -1338,7 +1315,6 @@ namespace Microsoft.AspNetCore.Http.Connections.Tests var manager = CreateConnectionManager(loggerFactory); var connection = manager.CreateConnection(); connection.TransportType = HttpTransportType.LongPolling; - connection.Items[ConnectionMetadataNames.Transport] = HttpTransportType.LongPolling; var dispatcher = new HttpConnectionDispatcher(manager, loggerFactory); var context = new DefaultHttpContext(); context.Features.Set(new ResponseFeature()); @@ -1396,7 +1372,6 @@ namespace Microsoft.AspNetCore.Http.Connections.Tests var manager = CreateConnectionManager(loggerFactory); var connection = manager.CreateConnection(); connection.TransportType = HttpTransportType.LongPolling; - connection.Items[ConnectionMetadataNames.Transport] = HttpTransportType.LongPolling; var dispatcher = new HttpConnectionDispatcher(manager, loggerFactory); var context = new DefaultHttpContext(); context.Features.Set(new ResponseFeature()); @@ -1479,7 +1454,6 @@ namespace Microsoft.AspNetCore.Http.Connections.Tests var manager = CreateConnectionManager(loggerFactory); var connection = manager.CreateConnection(); connection.TransportType = HttpTransportType.LongPolling; - connection.Items[ConnectionMetadataNames.Transport] = HttpTransportType.LongPolling; var dispatcher = new HttpConnectionDispatcher(manager, loggerFactory); var context = new DefaultHttpContext(); context.Features.Set(new ResponseFeature()); @@ -1538,7 +1512,6 @@ namespace Microsoft.AspNetCore.Http.Connections.Tests var manager = CreateConnectionManager(loggerFactory); var connection = manager.CreateConnection(); connection.TransportType = HttpTransportType.LongPolling; - connection.Items[ConnectionMetadataNames.Transport] = HttpTransportType.LongPolling; var dispatcher = new HttpConnectionDispatcher(manager, loggerFactory); var context = new DefaultHttpContext(); var services = new ServiceCollection(); @@ -1593,7 +1566,6 @@ namespace Microsoft.AspNetCore.Http.Connections.Tests var manager = CreateConnectionManager(loggerFactory); var connection = manager.CreateConnection(); connection.TransportType = HttpTransportType.LongPolling; - connection.Items[ConnectionMetadataNames.Transport] = HttpTransportType.LongPolling; var dispatcher = new HttpConnectionDispatcher(manager, loggerFactory); @@ -1690,7 +1662,7 @@ namespace Microsoft.AspNetCore.Http.Connections.Tests var manager = CreateConnectionManager(loggerFactory); var connection = manager.CreateConnection(); connection.TransportType = transportType; - connection.Items[ConnectionMetadataNames.Transport] = transportType; + var dispatcher = new HttpConnectionDispatcher(manager, loggerFactory); using (var strm = new MemoryStream()) { diff --git a/test/Microsoft.AspNetCore.SignalR.Client.FunctionalTests/Hubs.cs b/test/Microsoft.AspNetCore.SignalR.Client.FunctionalTests/Hubs.cs index ee7c962a45..03fc3ea45a 100644 --- a/test/Microsoft.AspNetCore.SignalR.Client.FunctionalTests/Hubs.cs +++ b/test/Microsoft.AspNetCore.SignalR.Client.FunctionalTests/Hubs.cs @@ -10,6 +10,7 @@ using System.Threading.Tasks; using Microsoft.AspNetCore.Authentication.JwtBearer; using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Http.Connections; +using Microsoft.AspNetCore.Http.Connections.Features; using Microsoft.AspNetCore.Http.Features; namespace Microsoft.AspNetCore.SignalR.Client.FunctionalTests @@ -77,7 +78,7 @@ namespace Microsoft.AspNetCore.SignalR.Client.FunctionalTests public string GetActiveTransportName() { - return Context.Items[ConnectionMetadataNames.Transport].ToString(); + return Context.Features.Get().TransportType.ToString(); } } diff --git a/test/Microsoft.AspNetCore.SignalR.Tests/HubConnectionHandlerTests.cs b/test/Microsoft.AspNetCore.SignalR.Tests/HubConnectionHandlerTests.cs index efa6cdb108..a2f3e77420 100644 --- a/test/Microsoft.AspNetCore.SignalR.Tests/HubConnectionHandlerTests.cs +++ b/test/Microsoft.AspNetCore.SignalR.Tests/HubConnectionHandlerTests.cs @@ -13,6 +13,7 @@ using MessagePack.Formatters; using Microsoft.AspNetCore.Connections; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Http.Connections; +using Microsoft.AspNetCore.Http.Connections.Features; using Microsoft.AspNetCore.SignalR.Internal; using Microsoft.AspNetCore.SignalR.Internal.Protocol; using Microsoft.Extensions.DependencyInjection; @@ -1760,7 +1761,11 @@ namespace Microsoft.AspNetCore.SignalR.Tests using (var client = new TestClient()) { var httpContext = new DefaultHttpContext(); - client.Connection.SetHttpContext(httpContext); + var feature = new TestHttpContextFeature + { + HttpContext = httpContext + }; + client.Connection.Features.Set(feature); var connectionHandlerTask = await client.ConnectAsync(connectionHandler); await client.Connected.OrTimeout(); @@ -1985,5 +1990,10 @@ namespace Microsoft.AspNetCore.SignalR.Tests yield return new[] { typeof(MethodHub) }; yield return new[] { typeof(HubT) }; } + + public class TestHttpContextFeature : IHttpContextFeature + { + public HttpContext HttpContext { get; set; } + } } }