From e4f5fef7ac8389dcc6825010ef9690babce70cee Mon Sep 17 00:00:00 2001 From: BrennanConroy Date: Tue, 12 Mar 2019 10:24:54 -0700 Subject: [PATCH] Fix race in LongPolling causing flaky tests (#8114) --- ...Microsoft.AspNetCore.Http.Connections.netcoreapp3.0.cs | 3 ++- .../src/Internal/HttpConnectionContext.cs | 8 +++++++- .../src/Internal/HttpConnectionDispatcher.cs | 8 +++++--- 3 files changed, 14 insertions(+), 5 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 178c863787..dc2d3b7d8c 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 @@ -91,7 +91,7 @@ namespace Microsoft.AspNetCore.Http.Connections.Internal public System.DateTime LastSeenUtc { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } } 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 { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } } + public Microsoft.AspNetCore.Http.Connections.Internal.HttpConnectionStatus Status { get { throw null; } 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 { } } @@ -102,6 +102,7 @@ namespace Microsoft.AspNetCore.Http.Connections.Internal public System.Threading.Tasks.Task DisposeAsync(bool closeGracefully = false) { throw null; } 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 partial class HttpConnectionDispatcher { diff --git a/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionContext.cs b/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionContext.cs index fd9d22490a..1962c3b348 100644 --- a/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionContext.cs +++ b/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionContext.cs @@ -35,6 +35,7 @@ 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 @@ -95,7 +96,7 @@ namespace Microsoft.AspNetCore.Http.Connections.Internal public DateTime LastSeenUtc { get; set; } - public HttpConnectionStatus Status { get; set; } = HttpConnectionStatus.Inactive; + public HttpConnectionStatus Status { get => (HttpConnectionStatus)_status; set => Interlocked.Exchange(ref _status, (int)value); } public override string ConnectionId { get; set; } @@ -309,6 +310,11 @@ namespace Microsoft.AspNetCore.Http.Connections.Internal } } + public bool TryChangeState(HttpConnectionStatus from, HttpConnectionStatus to) + { + return Interlocked.CompareExchange(ref _status, (int)to, (int)from) == (int)from; + } + private static class Log { private static readonly Action _disposingConnection = diff --git a/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs b/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs index 18ed1f1ee6..e64ee01c5a 100644 --- a/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs +++ b/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs @@ -235,7 +235,7 @@ namespace Microsoft.AspNetCore.Http.Connections.Internal } // Mark the connection as active - connection.Status = HttpConnectionStatus.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) @@ -331,7 +331,9 @@ namespace Microsoft.AspNetCore.Http.Connections.Internal // Mark the connection as inactive connection.LastSeenUtc = DateTime.UtcNow; - connection.Status = HttpConnectionStatus.Inactive; + // 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); } } finally @@ -371,7 +373,7 @@ namespace Microsoft.AspNetCore.Http.Connections.Internal } // Mark the connection as active - connection.Status = HttpConnectionStatus.Active; + connection.TryChangeState(HttpConnectionStatus.Inactive, HttpConnectionStatus.Active); // Call into the end point passing the connection connection.ApplicationTask = ExecuteApplication(connectionDelegate, connection);