From 13389732128a4cd74c34097f39d278e28c172011 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Thu, 14 Mar 2019 11:33:32 -0700 Subject: [PATCH] Better handle HttpConnectionContext state transitions (#8225) --- ...pNetCore.Http.Connections.netcoreapp3.0.cs | 11 +- .../src/Internal/HttpConnectionContext.cs | 185 ++++++++++++++++-- .../Internal/HttpConnectionDispatcher.Log.cs | 2 +- .../src/Internal/HttpConnectionDispatcher.cs | 178 +++-------------- .../src/Internal/HttpConnectionManager.cs | 24 +-- .../Transports/LongPollingTransport.cs | 2 +- .../test/HttpConnectionDispatcherTests.cs | 62 +++++- 7 files changed, 270 insertions(+), 194 deletions(-) diff --git a/src/SignalR/common/Http.Connections/ref/Microsoft.AspNetCore.Http.Connections.netcoreapp3.0.cs b/src/SignalR/common/Http.Connections/ref/Microsoft.AspNetCore.Http.Connections.netcoreapp3.0.cs index 3736fe523c..2aefc48b03 100644 --- a/src/SignalR/common/Http.Connections/ref/Microsoft.AspNetCore.Http.Connections.netcoreapp3.0.cs +++ b/src/SignalR/common/Http.Connections/ref/Microsoft.AspNetCore.Http.Connections.netcoreapp3.0.cs @@ -89,9 +89,9 @@ namespace Microsoft.AspNetCore.Http.Connections.Internal public Microsoft.AspNetCore.Http.HttpContext HttpContext { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } } public override System.Collections.Generic.IDictionary Items { get { throw null; } set { } } public System.DateTime LastSeenUtc { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } } + public System.DateTime? LastSeenUtcIfInactive { get { throw null; } } public System.Threading.Tasks.Task PreviousPollTask { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } } - public System.Threading.SemaphoreSlim StateLock { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } } - public Microsoft.AspNetCore.Http.Connections.Internal.HttpConnectionStatus Status { get { throw null; } set { } } + public Microsoft.AspNetCore.Http.Connections.Internal.HttpConnectionStatus Status { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } } public Microsoft.AspNetCore.Connections.TransferFormat SupportedFormats { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } } public override System.IO.Pipelines.IDuplexPipe Transport { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } } public System.Threading.Tasks.Task TransportTask { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } } @@ -100,9 +100,11 @@ namespace Microsoft.AspNetCore.Http.Connections.Internal public System.Threading.SemaphoreSlim WriteLock { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } } [System.Diagnostics.DebuggerStepThroughAttribute] public System.Threading.Tasks.Task DisposeAsync(bool closeGracefully = false) { throw null; } + public void MarkInactive() { } public void OnHeartbeat(System.Action action, object state) { } public void TickHeartbeat() { } - public bool TryChangeState(Microsoft.AspNetCore.Http.Connections.Internal.HttpConnectionStatus from, Microsoft.AspNetCore.Http.Connections.Internal.HttpConnectionStatus to) { throw null; } + public bool TryActivateLongPollingConnection(Microsoft.AspNetCore.Connections.ConnectionDelegate connectionDelegate, Microsoft.AspNetCore.Http.HttpContext nonClonedContext, System.TimeSpan pollTimeout, System.Threading.Tasks.Task currentRequestTask, Microsoft.Extensions.Logging.ILoggerFactory loggerFactory, Microsoft.Extensions.Logging.ILogger dispatcherLogger) { throw null; } + public bool TryActivatePersistentConnection(Microsoft.AspNetCore.Connections.ConnectionDelegate connectionDelegate, Microsoft.AspNetCore.Http.Connections.Internal.Transports.IHttpTransport transport, Microsoft.Extensions.Logging.ILogger dispatcherLogger) { throw null; } } public partial class HttpConnectionDispatcher { @@ -122,8 +124,7 @@ namespace Microsoft.AspNetCore.Http.Connections.Internal [System.Diagnostics.DebuggerStepThroughAttribute] public System.Threading.Tasks.Task DisposeAndRemoveAsync(Microsoft.AspNetCore.Http.Connections.Internal.HttpConnectionContext connection, bool closeGracefully) { throw null; } public void RemoveConnection(string id) { } - [System.Diagnostics.DebuggerStepThroughAttribute] - public System.Threading.Tasks.Task ScanAsync() { throw null; } + public void Scan() { } public void Start() { } public bool TryGetConnection(string id, out Microsoft.AspNetCore.Http.Connections.Internal.HttpConnectionContext connection) { throw null; } } diff --git a/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionContext.cs b/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionContext.cs index 1962c3b348..5556f76dd1 100644 --- a/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionContext.cs +++ b/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionContext.cs @@ -4,6 +4,7 @@ using System; using System.Collections.Concurrent; using System.Collections.Generic; +using System.Diagnostics; using System.IO.Pipelines; using System.Security.Claims; using System.Security.Principal; @@ -12,7 +13,9 @@ using System.Threading.Tasks; using Microsoft.AspNetCore.Connections; using Microsoft.AspNetCore.Connections.Features; using Microsoft.AspNetCore.Http.Connections.Features; +using Microsoft.AspNetCore.Http.Connections.Internal.Transports; using Microsoft.AspNetCore.Http.Features; +using Microsoft.AspNetCore.Internal; using Microsoft.Extensions.Logging; namespace Microsoft.AspNetCore.Http.Connections.Internal @@ -28,6 +31,7 @@ namespace Microsoft.AspNetCore.Http.Connections.Internal IHttpTransportFeature, IConnectionInherentKeepAliveFeature { + private readonly object _stateLock = new object(); private readonly object _itemsLock = new object(); private readonly object _heartbeatLock = new object(); private List<(Action handler, object state)> _heartbeatHandlers; @@ -35,7 +39,6 @@ namespace Microsoft.AspNetCore.Http.Connections.Internal private PipeWriterStream _applicationStream; private IDuplexPipe _application; private IDictionary _items; - private int _status = (int)HttpConnectionStatus.Inactive; // This tcs exists so that multiple calls to DisposeAsync all wait asynchronously // on the same task @@ -83,7 +86,6 @@ namespace Microsoft.AspNetCore.Http.Connections.Internal public HttpTransportType TransportType { get; set; } public SemaphoreSlim WriteLock { get; } = new SemaphoreSlim(1, 1); - public SemaphoreSlim StateLock { get; } = new SemaphoreSlim(1, 1); // Used for testing only internal Task DisposeAndRemoveTask { get; set; } @@ -96,7 +98,18 @@ namespace Microsoft.AspNetCore.Http.Connections.Internal public DateTime LastSeenUtc { get; set; } - public HttpConnectionStatus Status { get => (HttpConnectionStatus)_status; set => Interlocked.Exchange(ref _status, (int)value); } + public DateTime? LastSeenUtcIfInactive + { + get + { + lock (_stateLock) + { + return Status == HttpConnectionStatus.Inactive ? (DateTime?)LastSeenUtc : null; + } + } + } + + public HttpConnectionStatus Status { get; set; } = HttpConnectionStatus.Inactive; public override string ConnectionId { get; set; } @@ -184,29 +197,29 @@ namespace Microsoft.AspNetCore.Http.Connections.Internal { Task disposeTask; - await StateLock.WaitAsync(); try { - if (Status == HttpConnectionStatus.Disposed) + lock (_stateLock) { - disposeTask = _disposeTcs.Task; - } - else - { - Status = HttpConnectionStatus.Disposed; + if (Status == HttpConnectionStatus.Disposed) + { + disposeTask = _disposeTcs.Task; + } + else + { + Status = HttpConnectionStatus.Disposed; - Log.DisposingConnection(_logger, ConnectionId); + Log.DisposingConnection(_logger, ConnectionId); - var applicationTask = ApplicationTask ?? Task.CompletedTask; - var transportTask = TransportTask ?? Task.CompletedTask; + var applicationTask = ApplicationTask ?? Task.CompletedTask; + var transportTask = TransportTask ?? Task.CompletedTask; - disposeTask = WaitOnTasks(applicationTask, transportTask, closeGracefully); + disposeTask = WaitOnTasks(applicationTask, transportTask, closeGracefully); + } } } finally { - StateLock.Release(); - Cancellation?.Dispose(); Cancellation = null; @@ -310,9 +323,145 @@ namespace Microsoft.AspNetCore.Http.Connections.Internal } } - public bool TryChangeState(HttpConnectionStatus from, HttpConnectionStatus to) + public bool TryActivatePersistentConnection( + ConnectionDelegate connectionDelegate, + IHttpTransport transport, + ILogger dispatcherLogger) { - return Interlocked.CompareExchange(ref _status, (int)to, (int)from) == (int)from; + lock (_stateLock) + { + if (Status == HttpConnectionStatus.Inactive) + { + Status = HttpConnectionStatus.Active; + + // Call into the end point passing the connection + ApplicationTask = ExecuteApplication(connectionDelegate); + + // Start the transport + TransportTask = transport.ProcessRequestAsync(HttpContext, HttpContext.RequestAborted); + + return true; + } + else + { + FailActivationUnsynchronized(HttpContext, dispatcherLogger); + + return false; + } + } + } + + public bool TryActivateLongPollingConnection( + ConnectionDelegate connectionDelegate, + HttpContext nonClonedContext, + TimeSpan pollTimeout, + Task currentRequestTask, + ILoggerFactory loggerFactory, + ILogger dispatcherLogger) + { + lock (_stateLock) + { + if (Status == HttpConnectionStatus.Inactive) + { + Status = HttpConnectionStatus.Active; + + PreviousPollTask = currentRequestTask; + + // Raise OnConnected for new connections only since polls happen all the time + if (ApplicationTask == null) + { + HttpConnectionDispatcher.Log.EstablishedConnection(dispatcherLogger); + + ApplicationTask = ExecuteApplication(connectionDelegate); + + nonClonedContext.Response.ContentType = "application/octet-stream"; + + // This request has no content + nonClonedContext.Response.ContentLength = 0; + + // On the first poll, we flush the response immediately to mark the poll as "initialized" so future + // requests can be made safely + TransportTask = nonClonedContext.Response.Body.FlushAsync(); + } + else + { + HttpConnectionDispatcher.Log.ResumingConnection(dispatcherLogger); + + // REVIEW: Performance of this isn't great as this does a bunch of per request allocations + Cancellation = new CancellationTokenSource(); + + var timeoutSource = new CancellationTokenSource(); + var tokenSource = CancellationTokenSource.CreateLinkedTokenSource(Cancellation.Token, nonClonedContext.RequestAborted, timeoutSource.Token); + + // Dispose these tokens when the request is over + nonClonedContext.Response.RegisterForDispose(timeoutSource); + nonClonedContext.Response.RegisterForDispose(tokenSource); + + var longPolling = new LongPollingTransport(timeoutSource.Token, Application.Input, loggerFactory); + + // Start the transport + TransportTask = longPolling.ProcessRequestAsync(nonClonedContext, tokenSource.Token); + + // Start the timeout after we return from creating the transport task + timeoutSource.CancelAfter(pollTimeout); + } + + return true; + } + else + { + FailActivationUnsynchronized(nonClonedContext, dispatcherLogger); + + return false; + } + } + } + + private void FailActivationUnsynchronized(HttpContext nonClonedContext, ILogger dispatcherLogger) + { + if (Status == HttpConnectionStatus.Active) + { + HttpConnectionDispatcher.Log.ConnectionAlreadyActive(dispatcherLogger, ConnectionId, HttpContext.TraceIdentifier); + + // Reject the request with a 409 conflict + nonClonedContext.Response.StatusCode = StatusCodes.Status409Conflict; + nonClonedContext.Response.ContentType = "text/plain"; + } + else + { + Debug.Assert(Status == HttpConnectionStatus.Disposed); + + HttpConnectionDispatcher.Log.ConnectionDisposed(dispatcherLogger, ConnectionId); + + // Connection was disposed + nonClonedContext.Response.StatusCode = StatusCodes.Status404NotFound; + nonClonedContext.Response.ContentType = "text/plain"; + } + } + + public void MarkInactive() + { + lock (_stateLock) + { + if (Status == HttpConnectionStatus.Active) + { + Status = HttpConnectionStatus.Inactive; + LastSeenUtc = DateTime.UtcNow; + } + } + } + + private async Task ExecuteApplication(ConnectionDelegate connectionDelegate) + { + // Verify some initialization invariants + Debug.Assert(TransportType != HttpTransportType.None, "Transport has not been initialized yet"); + + // Jump onto the thread pool thread so blocking user code doesn't block the setup of the + // connection and transport + await AwaitableThreadPool.Yield(); + + // Running this in an async method turns sync exceptions into async ones + await connectionDelegate(this); } private static class Log diff --git a/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.Log.cs b/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.Log.cs index 799c43af43..8a013db4ad 100644 --- a/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.Log.cs +++ b/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.Log.cs @@ -8,7 +8,7 @@ namespace Microsoft.AspNetCore.Http.Connections.Internal { public partial class HttpConnectionDispatcher { - private static class Log + internal static class Log { private static readonly Action _connectionDisposed = LoggerMessage.Define(LogLevel.Debug, new EventId(1, "ConnectionDisposed"), "Connection {TransportConnectionId} was disposed."); diff --git a/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs b/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs index e64ee01c5a..a1790f9423 100644 --- a/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs +++ b/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs @@ -4,7 +4,6 @@ using System; using System.Buffers; using System.Collections.Generic; -using System.Diagnostics; using System.IO; using System.IO.Pipelines; using System.Security.Claims; @@ -194,99 +193,36 @@ namespace Microsoft.AspNetCore.Http.Connections.Internal // Create a new Tcs every poll to keep track of the poll finishing, so we can properly wait on previous polls var currentRequestTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); - await connection.StateLock.WaitAsync(); - try + using (connection.Cancellation) { - if (connection.Status == HttpConnectionStatus.Disposed) - { - Log.ConnectionDisposed(_logger, connection.ConnectionId); + // Cancel the previous request + connection.Cancellation?.Cancel(); - // The connection was disposed - context.Response.StatusCode = StatusCodes.Status404NotFound; + try + { + // Wait for the previous request to drain + await connection.PreviousPollTask; + } + catch (OperationCanceledException) + { + // Previous poll canceled due to connection closing, close this poll too context.Response.ContentType = "text/plain"; + context.Response.StatusCode = StatusCodes.Status204NoContent; return; } - - if (connection.Status == HttpConnectionStatus.Active) - { - var existing = connection.GetHttpContext(); - Log.ConnectionAlreadyActive(_logger, connection.ConnectionId, existing.TraceIdentifier); - } - - using (connection.Cancellation) - { - // Cancel the previous request - connection.Cancellation?.Cancel(); - - try - { - // Wait for the previous request to drain - await connection.PreviousPollTask; - } - catch (OperationCanceledException) - { - // Previous poll canceled due to connection closing, close this poll too - context.Response.ContentType = "text/plain"; - context.Response.StatusCode = StatusCodes.Status204NoContent; - return; - } - - connection.PreviousPollTask = currentRequestTcs.Task; - } - - // Mark the connection as active - connection.TryChangeState(from: HttpConnectionStatus.Inactive, to: HttpConnectionStatus.Active); - - // Raise OnConnected for new connections only since polls happen all the time - if (connection.ApplicationTask == null) - { - Log.EstablishedConnection(_logger); - - connection.ApplicationTask = ExecuteApplication(connectionDelegate, connection); - - context.Response.ContentType = "application/octet-stream"; - - // This request has no content - context.Response.ContentLength = 0; - - // On the first poll, we flush the response immediately to mark the poll as "initialized" so future - // requests can be made safely - connection.TransportTask = context.Response.Body.FlushAsync(); - } - else - { - Log.ResumingConnection(_logger); - - // REVIEW: Performance of this isn't great as this does a bunch of per request allocations - connection.Cancellation = new CancellationTokenSource(); - - var timeoutSource = new CancellationTokenSource(); - var tokenSource = CancellationTokenSource.CreateLinkedTokenSource(connection.Cancellation.Token, context.RequestAborted, timeoutSource.Token); - - // Dispose these tokens when the request is over - context.Response.RegisterForDispose(timeoutSource); - context.Response.RegisterForDispose(tokenSource); - - var longPolling = new LongPollingTransport(timeoutSource.Token, connection.Application.Input, _loggerFactory); - - // Start the transport - connection.TransportTask = longPolling.ProcessRequestAsync(context, tokenSource.Token); - - // Start the timeout after we return from creating the transport task - timeoutSource.CancelAfter(options.LongPolling.PollTimeout); - } } - finally + + if (!connection.TryActivateLongPollingConnection( + connectionDelegate, context, options.LongPolling.PollTimeout, + currentRequestTcs.Task, _loggerFactory, _logger)) { - connection.StateLock.Release(); + return; } var resultTask = await Task.WhenAny(connection.ApplicationTask, connection.TransportTask); try { - var pollAgain = true; - // If the application ended before the transport task then we potentially need to end the connection if (resultTask == connection.ApplicationTask) { @@ -305,9 +241,11 @@ namespace Microsoft.AspNetCore.Http.Connections.Internal // We should be able to safely dispose because there's no more data being written // We don't need to wait for close here since we've already waited for both sides await _manager.DisposeAndRemoveAsync(connection, closeGracefully: false); - - // Don't poll again if we've removed the connection completely - pollAgain = false; + } + else + { + // Only allow repoll if we aren't removing the connection. + connection.MarkInactive(); } } else if (resultTask.IsFaulted) @@ -317,23 +255,11 @@ namespace Microsoft.AspNetCore.Http.Connections.Internal // transport task was faulted, we should remove the connection await _manager.DisposeAndRemoveAsync(connection, closeGracefully: false); - - pollAgain = false; } - else if (context.Response.StatusCode == StatusCodes.Status204NoContent) + else { - // Don't poll if the transport task was canceled - pollAgain = false; - } - - if (pollAgain) - { - // Mark the connection as inactive - connection.LastSeenUtc = DateTime.UtcNow; - - // This is done outside a lock because the next poll might be waiting in the lock already and waiting for currentRequestTcs to complete - // A DELETE request could have set the status to Disposed. If that is the case we don't want to change the state ever. - connection.TryChangeState(from: HttpConnectionStatus.Active, to: HttpConnectionStatus.Inactive); + // Only allow repoll if we aren't removing the connection. + connection.MarkInactive(); } } finally @@ -350,59 +276,13 @@ namespace Microsoft.AspNetCore.Http.Connections.Internal HttpContext context, HttpConnectionContext connection) { - await connection.StateLock.WaitAsync(); - try + if (connection.TryActivatePersistentConnection(connectionDelegate, transport, _logger)) { - if (connection.Status == HttpConnectionStatus.Disposed) - { - Log.ConnectionDisposed(_logger, connection.ConnectionId); + // Wait for any of them to end + await Task.WhenAny(connection.ApplicationTask, connection.TransportTask); - // Connection was disposed - context.Response.StatusCode = StatusCodes.Status404NotFound; - return; - } - - // There's already an active request - if (connection.Status == HttpConnectionStatus.Active) - { - Log.ConnectionAlreadyActive(_logger, connection.ConnectionId, connection.GetHttpContext().TraceIdentifier); - - // Reject the request with a 409 conflict - context.Response.StatusCode = StatusCodes.Status409Conflict; - return; - } - - // Mark the connection as active - connection.TryChangeState(HttpConnectionStatus.Inactive, HttpConnectionStatus.Active); - - // Call into the end point passing the connection - connection.ApplicationTask = ExecuteApplication(connectionDelegate, connection); - - // Start the transport - connection.TransportTask = transport.ProcessRequestAsync(context, context.RequestAborted); + await _manager.DisposeAndRemoveAsync(connection, closeGracefully: true); } - finally - { - connection.StateLock.Release(); - } - - // Wait for any of them to end - await Task.WhenAny(connection.ApplicationTask, connection.TransportTask); - - await _manager.DisposeAndRemoveAsync(connection, closeGracefully: true); - } - - private async Task ExecuteApplication(ConnectionDelegate connectionDelegate, HttpConnectionContext connection) - { - // Verify some initialization invariants - Debug.Assert(connection.TransportType != HttpTransportType.None, "Transport has not been initialized yet"); - - // Jump onto the thread pool thread so blocking user code doesn't block the setup of the - // connection and transport - await AwaitableThreadPool.Yield(); - - // Running this in an async method turns sync exceptions into async ones - await connectionDelegate(connection); } private async Task ProcessNegotiate(HttpContext context, HttpConnectionDispatcherOptions options, ConnectionLogScope logScope) diff --git a/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionManager.cs b/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionManager.cs index 4d1dc43a2c..283bee8d98 100644 --- a/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionManager.cs +++ b/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionManager.cs @@ -125,7 +125,7 @@ namespace Microsoft.AspNetCore.Http.Connections.Internal { try { - await ScanAsync(); + Scan(); } catch (Exception ex) { @@ -137,32 +137,18 @@ namespace Microsoft.AspNetCore.Http.Connections.Internal Log.HeartBeatEnded(_logger); } - public async Task ScanAsync() + public void Scan() { // Scan the registered connections looking for ones that have timed out foreach (var c in _connections) { - HttpConnectionStatus status; - DateTimeOffset lastSeenUtc; var connection = c.Value.Connection; - - await connection.StateLock.WaitAsync(); - - try - { - // Capture the connection state - status = connection.Status; - - lastSeenUtc = connection.LastSeenUtc; - } - finally - { - connection.StateLock.Release(); - } + // Capture the connection state + var lastSeenUtc = connection.LastSeenUtcIfInactive; // Once the decision has been made to dispose we don't check the status again // But don't clean up connections while the debugger is attached. - if (!Debugger.IsAttached && status == HttpConnectionStatus.Inactive && (DateTimeOffset.UtcNow - lastSeenUtc).TotalSeconds > _disconnectTimeout.TotalSeconds) + if (!Debugger.IsAttached && lastSeenUtc.HasValue && (DateTimeOffset.UtcNow - lastSeenUtc.Value).TotalSeconds > _disconnectTimeout.TotalSeconds) { Log.ConnectionTimedOut(_logger, connection.ConnectionId); HttpConnectionsEventSource.Log.ConnectionTimedOut(connection.ConnectionId); diff --git a/src/SignalR/common/Http.Connections/src/Internal/Transports/LongPollingTransport.cs b/src/SignalR/common/Http.Connections/src/Internal/Transports/LongPollingTransport.cs index f3cd87f484..d14164b098 100644 --- a/src/SignalR/common/Http.Connections/src/Internal/Transports/LongPollingTransport.cs +++ b/src/SignalR/common/Http.Connections/src/Internal/Transports/LongPollingTransport.cs @@ -60,7 +60,7 @@ namespace Microsoft.AspNetCore.Http.Connections.Internal.Transports { // 3 cases: // 1 - Request aborted, the client disconnected (no response) - // 2 - The poll timeout is hit (204) + // 2 - The poll timeout is hit (200) // 3 - A new request comes in and cancels this request (204) // Case 1 diff --git a/src/SignalR/common/Http.Connections/test/HttpConnectionDispatcherTests.cs b/src/SignalR/common/Http.Connections/test/HttpConnectionDispatcherTests.cs index dcf72459d7..a328289a8b 100644 --- a/src/SignalR/common/Http.Connections/test/HttpConnectionDispatcherTests.cs +++ b/src/SignalR/common/Http.Connections/test/HttpConnectionDispatcherTests.cs @@ -444,7 +444,7 @@ namespace Microsoft.AspNetCore.Http.Connections.Tests // The application is still running here because the poll is only killed // by the heartbeat so we pretend to do a scan and this should force the application task to complete - await manager.ScanAsync(); + manager.Scan(); // The application task should complete gracefully await connection.ApplicationTask.OrTimeout(); @@ -1061,6 +1061,66 @@ namespace Microsoft.AspNetCore.Http.Connections.Tests } } + [Fact] + public async Task MultipleRequestsToActiveConnectionId409ForLongPolling() + { + using (StartVerifiableLog()) + { + var manager = CreateConnectionManager(LoggerFactory); + var connection = manager.CreateConnection(); + connection.TransportType = HttpTransportType.LongPolling; + + var dispatcher = new HttpConnectionDispatcher(manager, LoggerFactory); + + var context1 = MakeRequest("/foo", connection); + var context2 = MakeRequest("/foo", connection); + + var services = new ServiceCollection(); + services.AddSingleton(); + var builder = new ConnectionBuilder(services.BuildServiceProvider()); + builder.UseConnectionHandler(); + var app = builder.Build(); + var options = new HttpConnectionDispatcherOptions(); + + // Prime the polling. Expect any empty response showing the transport is initialized. + var request1 = dispatcher.ExecuteAsync(context1, options, app); + Assert.True(request1.IsCompleted); + + // Manually control PreviousPollTask instead of using a real PreviousPollTask, because a real + // PreviousPollTask might complete too early when the second request cancels it. + var lastPollTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + connection.PreviousPollTask = lastPollTcs.Task; + + request1 = dispatcher.ExecuteAsync(context1, options, app); + var request2 = dispatcher.ExecuteAsync(context2, options, app); + + Assert.False(request1.IsCompleted); + Assert.False(request2.IsCompleted); + + lastPollTcs.SetResult(null); + + var completedTask = await Task.WhenAny(request1, request2).OrTimeout(); + + if (completedTask == request1) + { + Assert.Equal(StatusCodes.Status409Conflict, context1.Response.StatusCode); + Assert.False(request2.IsCompleted); + } + else + { + Assert.Equal(StatusCodes.Status409Conflict, context2.Response.StatusCode); + Assert.False(request1.IsCompleted); + } + + Assert.Equal(HttpConnectionStatus.Active, connection.Status); + + manager.CloseConnections(); + + await request1.OrTimeout(); + await request2.OrTimeout(); + } + } + [Theory] [InlineData(HttpTransportType.ServerSentEvents)] [InlineData(HttpTransportType.LongPolling)]