Skip to content

Commit 155c0db

Browse files
JamesNKJamesNK
committed
Change connection start to happen when transport is selected
1 parent 9d34e67 commit 155c0db

File tree

5 files changed

+76
-86
lines changed

5 files changed

+76
-86
lines changed

‎src/SignalR/common/Http.Connections/src/Internal/HttpConnectionContext.cs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,8 @@ public HttpConnectionContext(string connectionId, string connectionToken, ILogge
114114

115115
public HttpTransportType TransportType { get; set; }
116116

117+
internal long StartTimestamp { get; set; }
118+
117119
public SemaphoreSlim WriteLock { get; } = new SemaphoreSlim(1, 1);
118120

119121
// Used for testing only

‎src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
// The .NET Foundation licenses this file to you under the MIT license.
33

44
using System.Buffers;
5+
using System.Diagnostics;
56
using System.Security.Claims;
67
using System.Security.Principal;
78
using Microsoft.AspNetCore.Authentication;
@@ -552,7 +553,14 @@ private async Task<bool> EnsureConnectionStateAsync(HttpConnectionContext connec
552553

553554
if (connection.TransportType == HttpTransportType.None)
554555
{
556+
if (HttpConnectionsEventSource.Log.IsEnabled() || connection.MetricsContext.ConnectionDurationEnabled)
557+
{
558+
connection.StartTimestamp = Stopwatch.GetTimestamp();
559+
}
560+
555561
connection.TransportType = transportType;
562+
563+
HttpConnectionsEventSource.Log.ConnectionStart(connection.ConnectionId);
556564
_metrics.ConnectionTransportStart(connection.MetricsContext, transportType);
557565
}
558566
else if (connection.TransportType != transportType)

‎src/SignalR/common/Http.Connections/src/Internal/HttpConnectionManager.cs

Lines changed: 18 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,8 @@ internal sealed partial class HttpConnectionManager
1919
// TODO: Consider making this configurable? At least for testing?
2020
private static readonly TimeSpan _heartbeatTickRate = TimeSpan.FromSeconds(1);
2121

22-
private readonly ConcurrentDictionary<string, (HttpConnectionContext Connection, long StartTimestamp)> _connections =
23-
new ConcurrentDictionary<string, (HttpConnectionContext Connection, long StartTimestamp)>(StringComparer.Ordinal);
22+
private readonly ConcurrentDictionary<string, HttpConnectionContext> _connections =
23+
new ConcurrentDictionary<string, HttpConnectionContext>(StringComparer.Ordinal);
2424
private readonly PeriodicTimer _nextHeartbeat;
2525
private readonly ILogger<HttpConnectionManager> _logger;
2626
private readonly ILogger<HttpConnectionContext> _connectionLogger;
@@ -48,14 +48,7 @@ public void Start()
4848

4949
internal bool TryGetConnection(string id, [NotNullWhen(true)] out HttpConnectionContext? connection)
5050
{
51-
connection = null;
52-
53-
if (_connections.TryGetValue(id, out var pair))
54-
{
55-
connection = pair.Connection;
56-
return true;
57-
}
58-
return false;
51+
return _connections.TryGetValue(id, out connection);
5952
}
6053

6154
internal HttpConnectionContext CreateConnection()
@@ -82,31 +75,32 @@ internal HttpConnectionContext CreateConnection(HttpConnectionDispatcherOptions
8275

8376
var metricsContext = _metrics.CreateContext();
8477

85-
var startTimestamp = HttpConnectionsEventSource.Log.IsEnabled() || metricsContext.ConnectionDurationEnabled
86-
? Stopwatch.GetTimestamp()
87-
: default;
88-
8978
Log.CreatedNewConnection(_logger, id);
90-
HttpConnectionsEventSource.Log.ConnectionStart(id);
9179

9280
var pair = CreateConnectionPair(options.TransportPipeOptions, options.AppPipeOptions);
9381
var connection = new HttpConnectionContext(id, connectionToken, _connectionLogger, metricsContext, pair.Application, pair.Transport, options, useAck);
9482

95-
_connections.TryAdd(connectionToken, (connection, startTimestamp));
83+
_connections.TryAdd(connectionToken, connection);
9684

9785
return connection;
9886
}
9987

10088
public void RemoveConnection(string id, HttpTransportType transportType, HttpConnectionStopStatus status)
10189
{
102-
if (_connections.TryRemove(id, out var pair))
90+
// Remove the connection completely
91+
if (_connections.TryRemove(id, out var connection))
10392
{
104-
// Remove the connection completely
105-
var currentTimestamp = (pair.StartTimestamp > 0) ? Stopwatch.GetTimestamp() : default;
93+
// A connection is considered started when the transport is negotated.
94+
// You can't stop something that hasn't started so only log connection stop events if there is a transport.
95+
if (connection.TransportType != HttpTransportType.None)
96+
{
97+
var currentTimestamp = (connection.StartTimestamp > 0) ? Stopwatch.GetTimestamp() : default;
98+
99+
HttpConnectionsEventSource.Log.ConnectionStop(id, connection.StartTimestamp, currentTimestamp);
100+
_metrics.TransportStop(connection.MetricsContext, transportType);
101+
_metrics.ConnectionStop(connection.MetricsContext, transportType, status, connection.StartTimestamp, currentTimestamp);
102+
}
106103

107-
HttpConnectionsEventSource.Log.ConnectionStop(id, pair.StartTimestamp, currentTimestamp);
108-
_metrics.TransportStop(pair.Connection.MetricsContext, transportType);
109-
_metrics.ConnectionStop(pair.Connection.MetricsContext, transportType, status, pair.StartTimestamp, currentTimestamp);
110104
Log.RemovedConnection(_logger, id);
111105
}
112106
}
@@ -152,7 +146,7 @@ public void Scan()
152146
// Scan the registered connections looking for ones that have timed out
153147
foreach (var c in _connections)
154148
{
155-
var connection = c.Value.Connection;
149+
var connection = c.Value;
156150
// Capture the connection state
157151
var lastSeenTick = connection.LastSeenTicksIfInactive;
158152

@@ -200,7 +194,7 @@ public void CloseConnections()
200194
foreach (var c in _connections)
201195
{
202196
// We're shutting down so don't wait for closing the application
203-
tasks.Add(DisposeAndRemoveAsync(c.Value.Connection, closeGracefully: false, HttpConnectionStopStatus.AppShutdown));
197+
tasks.Add(DisposeAndRemoveAsync(c.Value, closeGracefully: false, HttpConnectionStopStatus.AppShutdown));
204198
}
205199

206200
Task.WaitAll(tasks.ToArray(), TimeSpan.FromSeconds(5));

‎src/SignalR/common/Http.Connections/test/HttpConnectionDispatcherTests.cs

Lines changed: 48 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1121,21 +1121,60 @@ public async Task Metrics()
11211121
Assert.Collection(connectionDuration.GetMeasurements(), m => AssertDuration(m, HttpConnectionStopStatus.NormalClosure, HttpTransportType.LongPolling));
11221122
Assert.Collection(currentConnections.GetMeasurements(), m => AssertTransport(m, 1, HttpTransportType.LongPolling), m => AssertTransport(m, -1, HttpTransportType.LongPolling));
11231123
}
1124+
}
11241125

1125-
static void AssertTransport(Measurement<long> measurement, long expected, HttpTransportType transportType)
1126+
[Fact]
1127+
public async Task Metrics_ListenStartAfterConnection_Empty()
1128+
{
1129+
using (StartVerifiableLog())
11261130
{
1127-
Assert.Equal(expected, measurement.Value);
1128-
Assert.Equal(transportType.ToString(), (string)measurement.Tags.ToArray().Single(t => t.Key == "transport").Value);
1129-
}
1131+
var testMeterFactory = new TestMeterFactory();
1132+
var metrics = new HttpConnectionsMetrics(testMeterFactory);
1133+
var manager = CreateConnectionManager(LoggerFactory, metrics);
1134+
var connection = manager.CreateConnection();
11301135

1131-
static void AssertDuration(Measurement<double> measurement, HttpConnectionStopStatus status, HttpTransportType transportType)
1132-
{
1133-
Assert.True(measurement.Value > 0);
1134-
Assert.Equal(status.ToString(), (string)measurement.Tags.ToArray().Single(t => t.Key == "status").Value);
1135-
Assert.Equal(transportType.ToString(), (string)measurement.Tags.ToArray().Single(t => t.Key == "transport").Value);
1136+
var dispatcher = CreateDispatcher(manager, LoggerFactory, metrics);
1137+
1138+
var services = new ServiceCollection();
1139+
services.AddSingleton<ImmediatelyCompleteConnectionHandler>();
1140+
var context = MakeRequest("/foo", connection, services);
1141+
1142+
var builder = new ConnectionBuilder(services.BuildServiceProvider());
1143+
builder.UseConnectionHandler<ImmediatelyCompleteConnectionHandler>();
1144+
var app = builder.Build();
1145+
// First poll will 200
1146+
await dispatcher.ExecuteAsync(context, new HttpConnectionDispatcherOptions(), app);
1147+
Assert.Equal(StatusCodes.Status200OK, context.Response.StatusCode);
1148+
1149+
using var connectionDuration = new InstrumentRecorder<double>(testMeterFactory, HttpConnectionsMetrics.MeterName, "signalr-http-transport-connection-duration");
1150+
using var currentConnections = new InstrumentRecorder<long>(testMeterFactory, HttpConnectionsMetrics.MeterName, "signalr-http-transport-current-connections");
1151+
1152+
await dispatcher.ExecuteAsync(context, new HttpConnectionDispatcherOptions(), app);
1153+
1154+
Assert.Equal(StatusCodes.Status204NoContent, context.Response.StatusCode);
1155+
AssertResponseHasCacheHeaders(context.Response);
1156+
1157+
var exists = manager.TryGetConnection(connection.ConnectionId, out _);
1158+
Assert.False(exists);
1159+
1160+
Assert.Empty(currentConnections.GetMeasurements());
1161+
Assert.Empty(connectionDuration.GetMeasurements());
11361162
}
11371163
}
11381164

1165+
private static void AssertTransport(Measurement<long> measurement, long expected, HttpTransportType transportType)
1166+
{
1167+
Assert.Equal(expected, measurement.Value);
1168+
Assert.Equal(transportType.ToString(), (string)measurement.Tags.ToArray().Single(t => t.Key == "transport").Value);
1169+
}
1170+
1171+
private static void AssertDuration(Measurement<double> measurement, HttpConnectionStopStatus status, HttpTransportType transportType)
1172+
{
1173+
Assert.True(measurement.Value > 0);
1174+
Assert.Equal(status.ToString(), (string)measurement.Tags.ToArray().Single(t => t.Key == "status").Value);
1175+
Assert.Equal(transportType.ToString(), (string)measurement.Tags.ToArray().Single(t => t.Key == "transport").Value);
1176+
}
1177+
11391178
[Fact]
11401179
public async Task LongPollingTimeoutSets200StatusCode()
11411180
{

‎src/SignalR/common/Http.Connections/test/HttpConnectionManagerTests.cs

Lines changed: 0 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -425,59 +425,6 @@ public async Task ApplicationLifetimeCanStartBeforeHttpConnectionManagerInitiali
425425
}
426426
}
427427

428-
[Fact]
429-
public void Metrics()
430-
{
431-
using (StartVerifiableLog())
432-
{
433-
var testMeterFactory = new TestMeterFactory();
434-
using var connectionDuration = new InstrumentRecorder<double>(testMeterFactory, HttpConnectionsMetrics.MeterName, "signalr-http-transport-connection-duration");
435-
436-
var connectionManager = CreateConnectionManager(LoggerFactory, metrics: new HttpConnectionsMetrics(testMeterFactory));
437-
var connection = connectionManager.CreateConnection();
438-
439-
Assert.NotNull(connection.ConnectionId);
440-
441-
Assert.Empty(connectionDuration.GetMeasurements());
442-
443-
connection.TransportType = HttpTransportType.WebSockets;
444-
445-
connectionManager.RemoveConnection(connection.ConnectionId, connection.TransportType, HttpConnectionStopStatus.NormalClosure);
446-
447-
Assert.Collection(connectionDuration.GetMeasurements(), m => AssertDuration(m, HttpConnectionStopStatus.NormalClosure, HttpTransportType.WebSockets));
448-
}
449-
}
450-
451-
[Fact]
452-
public void Metrics_ListenStartAfterConnection_Empty()
453-
{
454-
using (StartVerifiableLog())
455-
{
456-
var testMeterFactory = new TestMeterFactory();
457-
var connectionManager = CreateConnectionManager(LoggerFactory, metrics: new HttpConnectionsMetrics(testMeterFactory));
458-
var connection = connectionManager.CreateConnection();
459-
460-
using var connectionDuration = new InstrumentRecorder<double>(testMeterFactory, HttpConnectionsMetrics.MeterName, "http-server-connection-duration");
461-
using var currentConnections = new InstrumentRecorder<long>(testMeterFactory, HttpConnectionsMetrics.MeterName, "http-server-current-connections");
462-
463-
Assert.NotNull(connection.ConnectionId);
464-
465-
connection.TransportType = HttpTransportType.WebSockets;
466-
467-
connectionManager.RemoveConnection(connection.ConnectionId, connection.TransportType, HttpConnectionStopStatus.NormalClosure);
468-
469-
Assert.Empty(currentConnections.GetMeasurements());
470-
Assert.Empty(connectionDuration.GetMeasurements());
471-
}
472-
}
473-
474-
private static void AssertDuration(Measurement<double> measurement, HttpConnectionStopStatus status, HttpTransportType transportType)
475-
{
476-
Assert.True(measurement.Value > 0);
477-
Assert.Equal(status.ToString(), (string)measurement.Tags.ToArray().Single(t => t.Key == "status").Value);
478-
Assert.Equal(transportType.ToString(), (string)measurement.Tags.ToArray().Single(t => t.Key == "transport").Value);
479-
}
480-
481428
private static HttpConnectionManager CreateConnectionManager(ILoggerFactory loggerFactory, IHostApplicationLifetime lifetime = null, HttpConnectionsMetrics metrics = null)
482429
{
483430
lifetime ??= new EmptyApplicationLifetime();

0 commit comments

Comments
 (0)