Fix race in LongPolling causing flaky tests (#8114)

Author: BrennanConroy

src/SignalR/common/Http.Connections/ref/Microsoft.AspNetCore.Http.Connections.netcoreapp3.0.cs

@@ -91,7 +91,7 @@ public HttpConnectionContext(string id, System.IO.Pipelines.IDuplexPipe transpor
         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 @@ public HttpConnectionContext(string id, System.IO.Pipelines.IDuplexPipe transpor
         public System.Threading.Tasks.Task DisposeAsync(bool closeGracefully = false) { throw null; }
         public void OnHeartbeat(System.Action<object> 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
     {

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

@@ -35,6 +35,7 @@ public class HttpConnectionContext : ConnectionContext,
         private PipeWriterStream _applicationStream;
         private IDuplexPipe _application;
         private IDictionary<object, object> _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 @@ public HttpConnectionContext(string id, IDuplexPipe transport, IDuplexPipe appli
 
         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 @@ private async Task WaitOnTasks(Task applicationTask, Task transportTask, bool cl
             }
         }
 
+        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<ILogger, string, Exception> _disposingConnection =

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

@@ -235,7 +235,7 @@ private async Task ExecuteAsync(HttpContext context, ConnectionDelegate connecti
                     }
 
                     // 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 @@ private async Task ExecuteAsync(HttpContext context, ConnectionDelegate connecti
                         // 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 @@ private async Task DoPersistentConnection(ConnectionDelegate connectionDelegate,
                 }
 
                 // 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);