From 155c0db901aba9145179887993d5f59c18dceee7 Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Tue, 13 Jun 2023 16:58:03 +0800 Subject: [PATCH 1/2] Change connection start to happen when transport is selected --- .../src/Internal/HttpConnectionContext.cs | 2 + .../src/Internal/HttpConnectionDispatcher.cs | 8 +++ .../src/Internal/HttpConnectionManager.cs | 42 ++++++-------- .../test/HttpConnectionDispatcherTests.cs | 57 ++++++++++++++++--- .../test/HttpConnectionManagerTests.cs | 53 ----------------- 5 files changed, 76 insertions(+), 86 deletions(-) diff --git a/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionContext.cs b/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionContext.cs index 8ec21012979f..4508e36696d1 100644 --- a/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionContext.cs +++ b/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionContext.cs @@ -114,6 +114,8 @@ public HttpConnectionContext(string connectionId, string connectionToken, ILogge public HttpTransportType TransportType { get; set; } + internal long StartTimestamp { get; set; } + public SemaphoreSlim WriteLock { get; } = new SemaphoreSlim(1, 1); // Used for testing only diff --git a/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs b/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs index 983c667bf775..d4e3d75d62b2 100644 --- a/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs +++ b/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Buffers; +using System.Diagnostics; using System.Security.Claims; using System.Security.Principal; using Microsoft.AspNetCore.Authentication; @@ -552,7 +553,14 @@ private async Task EnsureConnectionStateAsync(HttpConnectionContext connec if (connection.TransportType == HttpTransportType.None) { + if (HttpConnectionsEventSource.Log.IsEnabled() || connection.MetricsContext.ConnectionDurationEnabled) + { + connection.StartTimestamp = Stopwatch.GetTimestamp(); + } + connection.TransportType = transportType; + + HttpConnectionsEventSource.Log.ConnectionStart(connection.ConnectionId); _metrics.ConnectionTransportStart(connection.MetricsContext, transportType); } else if (connection.TransportType != transportType) diff --git a/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionManager.cs b/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionManager.cs index 0c07cffb1728..389cb102c80f 100644 --- a/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionManager.cs +++ b/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionManager.cs @@ -19,8 +19,8 @@ internal sealed partial class HttpConnectionManager // TODO: Consider making this configurable? At least for testing? private static readonly TimeSpan _heartbeatTickRate = TimeSpan.FromSeconds(1); - private readonly ConcurrentDictionary _connections = - new ConcurrentDictionary(StringComparer.Ordinal); + private readonly ConcurrentDictionary _connections = + new ConcurrentDictionary(StringComparer.Ordinal); private readonly PeriodicTimer _nextHeartbeat; private readonly ILogger _logger; private readonly ILogger _connectionLogger; @@ -48,14 +48,7 @@ public void Start() internal bool TryGetConnection(string id, [NotNullWhen(true)] out HttpConnectionContext? connection) { - connection = null; - - if (_connections.TryGetValue(id, out var pair)) - { - connection = pair.Connection; - return true; - } - return false; + return _connections.TryGetValue(id, out connection); } internal HttpConnectionContext CreateConnection() @@ -82,31 +75,32 @@ internal HttpConnectionContext CreateConnection(HttpConnectionDispatcherOptions var metricsContext = _metrics.CreateContext(); - var startTimestamp = HttpConnectionsEventSource.Log.IsEnabled() || metricsContext.ConnectionDurationEnabled - ? Stopwatch.GetTimestamp() - : default; - Log.CreatedNewConnection(_logger, id); - HttpConnectionsEventSource.Log.ConnectionStart(id); var pair = CreateConnectionPair(options.TransportPipeOptions, options.AppPipeOptions); var connection = new HttpConnectionContext(id, connectionToken, _connectionLogger, metricsContext, pair.Application, pair.Transport, options, useAck); - _connections.TryAdd(connectionToken, (connection, startTimestamp)); + _connections.TryAdd(connectionToken, connection); return connection; } public void RemoveConnection(string id, HttpTransportType transportType, HttpConnectionStopStatus status) { - if (_connections.TryRemove(id, out var pair)) + // Remove the connection completely + if (_connections.TryRemove(id, out var connection)) { - // Remove the connection completely - var currentTimestamp = (pair.StartTimestamp > 0) ? Stopwatch.GetTimestamp() : default; + // A connection is considered started when the transport is negotated. + // You can't stop something that hasn't started so only log connection stop events if there is a transport. + if (connection.TransportType != HttpTransportType.None) + { + var currentTimestamp = (connection.StartTimestamp > 0) ? Stopwatch.GetTimestamp() : default; + + HttpConnectionsEventSource.Log.ConnectionStop(id, connection.StartTimestamp, currentTimestamp); + _metrics.TransportStop(connection.MetricsContext, transportType); + _metrics.ConnectionStop(connection.MetricsContext, transportType, status, connection.StartTimestamp, currentTimestamp); + } - HttpConnectionsEventSource.Log.ConnectionStop(id, pair.StartTimestamp, currentTimestamp); - _metrics.TransportStop(pair.Connection.MetricsContext, transportType); - _metrics.ConnectionStop(pair.Connection.MetricsContext, transportType, status, pair.StartTimestamp, currentTimestamp); Log.RemovedConnection(_logger, id); } } @@ -152,7 +146,7 @@ public void Scan() // Scan the registered connections looking for ones that have timed out foreach (var c in _connections) { - var connection = c.Value.Connection; + var connection = c.Value; // Capture the connection state var lastSeenTick = connection.LastSeenTicksIfInactive; @@ -200,7 +194,7 @@ public void CloseConnections() foreach (var c in _connections) { // We're shutting down so don't wait for closing the application - tasks.Add(DisposeAndRemoveAsync(c.Value.Connection, closeGracefully: false, HttpConnectionStopStatus.AppShutdown)); + tasks.Add(DisposeAndRemoveAsync(c.Value, closeGracefully: false, HttpConnectionStopStatus.AppShutdown)); } Task.WaitAll(tasks.ToArray(), TimeSpan.FromSeconds(5)); diff --git a/src/SignalR/common/Http.Connections/test/HttpConnectionDispatcherTests.cs b/src/SignalR/common/Http.Connections/test/HttpConnectionDispatcherTests.cs index 9de987aebea9..b810476a85fe 100644 --- a/src/SignalR/common/Http.Connections/test/HttpConnectionDispatcherTests.cs +++ b/src/SignalR/common/Http.Connections/test/HttpConnectionDispatcherTests.cs @@ -1121,21 +1121,60 @@ public async Task Metrics() Assert.Collection(connectionDuration.GetMeasurements(), m => AssertDuration(m, HttpConnectionStopStatus.NormalClosure, HttpTransportType.LongPolling)); Assert.Collection(currentConnections.GetMeasurements(), m => AssertTransport(m, 1, HttpTransportType.LongPolling), m => AssertTransport(m, -1, HttpTransportType.LongPolling)); } + } - static void AssertTransport(Measurement measurement, long expected, HttpTransportType transportType) + [Fact] + public async Task Metrics_ListenStartAfterConnection_Empty() + { + using (StartVerifiableLog()) { - Assert.Equal(expected, measurement.Value); - Assert.Equal(transportType.ToString(), (string)measurement.Tags.ToArray().Single(t => t.Key == "transport").Value); - } + var testMeterFactory = new TestMeterFactory(); + var metrics = new HttpConnectionsMetrics(testMeterFactory); + var manager = CreateConnectionManager(LoggerFactory, metrics); + var connection = manager.CreateConnection(); - static void AssertDuration(Measurement measurement, HttpConnectionStopStatus status, HttpTransportType transportType) - { - Assert.True(measurement.Value > 0); - Assert.Equal(status.ToString(), (string)measurement.Tags.ToArray().Single(t => t.Key == "status").Value); - Assert.Equal(transportType.ToString(), (string)measurement.Tags.ToArray().Single(t => t.Key == "transport").Value); + var dispatcher = CreateDispatcher(manager, LoggerFactory, metrics); + + var services = new ServiceCollection(); + services.AddSingleton(); + var context = MakeRequest("/foo", connection, services); + + var builder = new ConnectionBuilder(services.BuildServiceProvider()); + builder.UseConnectionHandler(); + var app = builder.Build(); + // First poll will 200 + await dispatcher.ExecuteAsync(context, new HttpConnectionDispatcherOptions(), app); + Assert.Equal(StatusCodes.Status200OK, context.Response.StatusCode); + + using var connectionDuration = new InstrumentRecorder(testMeterFactory, HttpConnectionsMetrics.MeterName, "signalr-http-transport-connection-duration"); + using var currentConnections = new InstrumentRecorder(testMeterFactory, HttpConnectionsMetrics.MeterName, "signalr-http-transport-current-connections"); + + await dispatcher.ExecuteAsync(context, new HttpConnectionDispatcherOptions(), app); + + Assert.Equal(StatusCodes.Status204NoContent, context.Response.StatusCode); + AssertResponseHasCacheHeaders(context.Response); + + var exists = manager.TryGetConnection(connection.ConnectionId, out _); + Assert.False(exists); + + Assert.Empty(currentConnections.GetMeasurements()); + Assert.Empty(connectionDuration.GetMeasurements()); } } + private static void AssertTransport(Measurement measurement, long expected, HttpTransportType transportType) + { + Assert.Equal(expected, measurement.Value); + Assert.Equal(transportType.ToString(), (string)measurement.Tags.ToArray().Single(t => t.Key == "transport").Value); + } + + private static void AssertDuration(Measurement measurement, HttpConnectionStopStatus status, HttpTransportType transportType) + { + Assert.True(measurement.Value > 0); + Assert.Equal(status.ToString(), (string)measurement.Tags.ToArray().Single(t => t.Key == "status").Value); + Assert.Equal(transportType.ToString(), (string)measurement.Tags.ToArray().Single(t => t.Key == "transport").Value); + } + [Fact] public async Task LongPollingTimeoutSets200StatusCode() { diff --git a/src/SignalR/common/Http.Connections/test/HttpConnectionManagerTests.cs b/src/SignalR/common/Http.Connections/test/HttpConnectionManagerTests.cs index 2802b34eb8fa..fafcd8c43f30 100644 --- a/src/SignalR/common/Http.Connections/test/HttpConnectionManagerTests.cs +++ b/src/SignalR/common/Http.Connections/test/HttpConnectionManagerTests.cs @@ -425,59 +425,6 @@ public async Task ApplicationLifetimeCanStartBeforeHttpConnectionManagerInitiali } } - [Fact] - public void Metrics() - { - using (StartVerifiableLog()) - { - var testMeterFactory = new TestMeterFactory(); - using var connectionDuration = new InstrumentRecorder(testMeterFactory, HttpConnectionsMetrics.MeterName, "signalr-http-transport-connection-duration"); - - var connectionManager = CreateConnectionManager(LoggerFactory, metrics: new HttpConnectionsMetrics(testMeterFactory)); - var connection = connectionManager.CreateConnection(); - - Assert.NotNull(connection.ConnectionId); - - Assert.Empty(connectionDuration.GetMeasurements()); - - connection.TransportType = HttpTransportType.WebSockets; - - connectionManager.RemoveConnection(connection.ConnectionId, connection.TransportType, HttpConnectionStopStatus.NormalClosure); - - Assert.Collection(connectionDuration.GetMeasurements(), m => AssertDuration(m, HttpConnectionStopStatus.NormalClosure, HttpTransportType.WebSockets)); - } - } - - [Fact] - public void Metrics_ListenStartAfterConnection_Empty() - { - using (StartVerifiableLog()) - { - var testMeterFactory = new TestMeterFactory(); - var connectionManager = CreateConnectionManager(LoggerFactory, metrics: new HttpConnectionsMetrics(testMeterFactory)); - var connection = connectionManager.CreateConnection(); - - using var connectionDuration = new InstrumentRecorder(testMeterFactory, HttpConnectionsMetrics.MeterName, "http-server-connection-duration"); - using var currentConnections = new InstrumentRecorder(testMeterFactory, HttpConnectionsMetrics.MeterName, "http-server-current-connections"); - - Assert.NotNull(connection.ConnectionId); - - connection.TransportType = HttpTransportType.WebSockets; - - connectionManager.RemoveConnection(connection.ConnectionId, connection.TransportType, HttpConnectionStopStatus.NormalClosure); - - Assert.Empty(currentConnections.GetMeasurements()); - Assert.Empty(connectionDuration.GetMeasurements()); - } - } - - private static void AssertDuration(Measurement measurement, HttpConnectionStopStatus status, HttpTransportType transportType) - { - Assert.True(measurement.Value > 0); - Assert.Equal(status.ToString(), (string)measurement.Tags.ToArray().Single(t => t.Key == "status").Value); - Assert.Equal(transportType.ToString(), (string)measurement.Tags.ToArray().Single(t => t.Key == "transport").Value); - } - private static HttpConnectionManager CreateConnectionManager(ILoggerFactory loggerFactory, IHostApplicationLifetime lifetime = null, HttpConnectionsMetrics metrics = null) { lifetime ??= new EmptyApplicationLifetime(); From 6cc56f6f2673b519232e8d777a718cb869f580d8 Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Wed, 14 Jun 2023 05:47:12 +0800 Subject: [PATCH 2/2] Update src/SignalR/common/Http.Connections/src/Internal/HttpConnectionManager.cs Co-authored-by: Brennan --- .../Http.Connections/src/Internal/HttpConnectionManager.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionManager.cs b/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionManager.cs index 389cb102c80f..c9117e0c0c1e 100644 --- a/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionManager.cs +++ b/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionManager.cs @@ -90,7 +90,7 @@ public void RemoveConnection(string id, HttpTransportType transportType, HttpCon // Remove the connection completely if (_connections.TryRemove(id, out var connection)) { - // A connection is considered started when the transport is negotated. + // A connection is considered started when the transport is negotiated. // You can't stop something that hasn't started so only log connection stop events if there is a transport. if (connection.TransportType != HttpTransportType.None) {