Remove QueuePolicy locks (#23187)

* Use field rather than property

Operate on a field directly, rather than through a property.

* Make field readonly

Make field read-only as its value is never changed.

* Remove lock

Use Interlocked.Decrement() instead of taking a lock.

* Remove lock for increment

This removes the need for the lock in the success case at the cost of an extra  Interlocked.Decrement() call for the failed case.

* Fix typos

Change "availible" to "available".

* Add unit test for full queue

Add a unit test that validates request is not queued if the queue is already full.

Author: martincostello

src/Middleware/ConcurrencyLimiter/src/ConcurrencyLimiterMiddleware.cs

@@ -96,7 +96,7 @@ private static class ConcurrencyLimiterLog
                 LoggerMessage.Define<int>(LogLevel.Debug, new EventId(3, "RequestRunImmediately"), "Below MaxConcurrentRequests limit, running request immediately. Current active requests: {ActiveRequests}");
 
             private static readonly Action<ILogger, Exception> _requestRejectedQueueFull =
-                LoggerMessage.Define(LogLevel.Debug, new EventId(4, "RequestRejectedQueueFull"), "Currently at the 'RequestQueueLimit', rejecting this request with a '503 server not availible' error");
+                LoggerMessage.Define(LogLevel.Debug, new EventId(4, "RequestRejectedQueueFull"), "Currently at the 'RequestQueueLimit', rejecting this request with a '503 server not available' error");
 
             internal static void RequestEnqueued(ILogger logger, int activeRequests)
             {

src/Middleware/ConcurrencyLimiter/src/QueuePolicies/QueuePolicy.cs

@@ -13,9 +13,9 @@ internal class QueuePolicy : IQueuePolicy, IDisposable
         private readonly int _maxTotalRequest;
         private readonly SemaphoreSlim _serverSemaphore;
 
-        private object _totalRequestsLock = new object();
+        private int _totalRequests;
 
-        public int TotalRequests { get; private set; }
+        public int TotalRequests => _totalRequests;
 
         public QueuePolicy(IOptions<QueuePolicyOptions> options)
         {
@@ -44,14 +44,12 @@ public ValueTask<bool> TryEnterAsync()
             // a return value of 'true' indicates that the request may proceed
             // _serverSemaphore.Release is *not* called in this method, it is called externally when requests leave the server
 
-            lock (_totalRequestsLock)
-            {
-                if (TotalRequests >= _maxTotalRequest)
-                {
-                    return new ValueTask<bool>(false);
-                }
+            int totalRequests = Interlocked.Increment(ref _totalRequests);
 
-                TotalRequests++;
+            if (totalRequests > _maxTotalRequest)
+            {
+                Interlocked.Decrement(ref _totalRequests);
+                return new ValueTask<bool>(false);
             }
 
             Task task = _serverSemaphore.WaitAsync();
@@ -67,10 +65,7 @@ public void OnExit()
         {
             _serverSemaphore.Release();
 
-            lock (_totalRequestsLock)
-            {
-                TotalRequests--;
-            }
+            Interlocked.Decrement(ref _totalRequests);
         }
 
         public void Dispose()

src/Middleware/ConcurrencyLimiter/src/QueuePolicies/QueuePolicyOptions.cs

@@ -15,7 +15,7 @@ public class QueuePolicyOptions
         public int MaxConcurrentRequests { get; set; }
 
         /// <summary>
-        /// Maximum number of queued requests before the server starts rejecting connections with '503 Service Unavailible'.
+        /// Maximum number of queued requests before the server starts rejecting connections with '503 Service Unavailable'.
         /// This option is highly application dependant, and must be configured by the application.
         /// </summary>
         public int RequestQueueLimit { get; set; }

src/Middleware/ConcurrencyLimiter/test/PolicyTests/QueuePolicyTests.cs

@@ -9,7 +9,7 @@ namespace Microsoft.AspNetCore.ConcurrencyLimiter.Tests.PolicyTests
     public class QueuePolicyTests
     {
         [Fact]
-        public void DoesNotWaitIfSpaceAvailible()
+        public void DoesNotWaitIfSpaceAvailable()
         {
             using var s = TestUtils.CreateQueuePolicy(2);
 
@@ -24,7 +24,7 @@ public void DoesNotWaitIfSpaceAvailible()
         }
 
         [Fact]
-        public async Task WaitsIfNoSpaceAvailible()
+        public async Task WaitsIfNoSpaceAvailable()
         {
             using var s = TestUtils.CreateQueuePolicy(1);
             Assert.True(await s.TryEnterAsync().OrTimeout());
@@ -36,6 +36,27 @@ public async Task WaitsIfNoSpaceAvailible()
             Assert.True(await waitingTask.OrTimeout());
         }
 
+        [Fact]
+        public void DoesNotWaitIfQueueFull()
+        {
+            using var s = TestUtils.CreateQueuePolicy(2, 1);
+
+            var t1 = s.TryEnterAsync();
+            Assert.True(t1.IsCompleted);
+            Assert.True(t1.Result);
+
+            var t2 = s.TryEnterAsync();
+            Assert.True(t2.IsCompleted);
+            Assert.True(t2.Result);
+
+            var t3 = s.TryEnterAsync();
+            Assert.False(t3.IsCompleted);
+
+            var t4 = s.TryEnterAsync();
+            Assert.True(t4.IsCompleted);
+            Assert.False(t4.Result);
+        }
+
         [Fact]
         public async Task IsEncapsulated()
         {