API review feedback (#1974)

- Remove SetHttpContext
- Remove HttpContextFeature
- Add and implement IHttpTransportFeature on HttpConnectionContext
- Remove ConnectionMetadataNames

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<IHttpTransportFeature>().TransportType.ToString();
         }
 
         public ComplexObject EchoComplexObject(ComplexObject complexObject)

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<IHttpTransportFeature>()?.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})");
             }
         }
 

src/Microsoft.AspNetCore.Http.Connections/ConnectionMetadataNames.cs

@@ -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);
-    }
-}

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; }
-    }
 }

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; }
+    }
+}

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<object> handler, object state)> _heartbeatHandlers;
@@ -62,6 +63,7 @@ namespace Microsoft.AspNetCore.Http.Connections
             Features.Set<IConnectionHeartbeatFeature>(this);
             Features.Set<ITransferFormatFeature>(this);
             Features.Set<IHttpContextFeature>(this);
+            Features.Set<IHttpTransportFeature>(this);
         }
 
         public HttpConnectionContext(string id, IDuplexPipe transport, IDuplexPipe application, ILogger logger = null)

src/Microsoft.AspNetCore.Http.Connections/HttpConnectionContextExtensions.cs

@@ -13,16 +13,5 @@ namespace Microsoft.AspNetCore.Http.Connections
         {
             return connection.Features.Get<IHttpContextFeature>()?.HttpContext;
         }
-
-        public static void SetHttpContext(this ConnectionContext connection, HttpContext httpContext)
-        {
-            var feature = connection.Features.Get<IHttpContextFeature>();
-            if (feature == null)
-            {
-                feature = new HttpContextFeature();
-                connection.Features.Set(feature);
-            }
-            feature.HttpContext = httpContext;
-        }
     }
 }

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)
             {

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<IHttpResponseFeature>(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<IHttpResponseFeature>(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<IHttpResponseFeature>(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())
             {

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<IHttpTransportFeature>().TransportType.ToString();
         }
     }
 

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<IHttpContextFeature>(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; }
+        }
     }
 }