From 451680c06bf2ebb9665076b7c8016a95fb320748 Mon Sep 17 00:00:00 2001
From: Albie <albion@dragonfruit.network>
Date: Wed, 23 Dec 2020 09:03:11 +0000
Subject: [PATCH] add read-write lock to replace loops

---
 DragonFruit.Common.Data/ApiClient.cs          | 94 +++++++++----------
 .../Headers/HeaderCollection.cs               |  3 +-
 2 files changed, 46 insertions(+), 51 deletions(-)

diff --git a/DragonFruit.Common.Data/ApiClient.cs b/DragonFruit.Common.Data/ApiClient.cs
index db38b05..2f0d65a 100644
--- a/DragonFruit.Common.Data/ApiClient.cs
+++ b/DragonFruit.Common.Data/ApiClient.cs
@@ -6,6 +6,7 @@
 using System.IO;
 using System.Linq;
 using System.Net.Http;
+using System.Runtime.CompilerServices;
 using System.Threading;
 using System.Threading.Tasks;
 using DragonFruit.Common.Data.Exceptions;
@@ -45,11 +46,15 @@ public ApiClient(ISerializer serializer)
             Serializer = serializer;
             Headers = new HeaderCollection(this);
 
+            _lock = new ReaderWriterLockSlim(LockRecursionPolicy.NoRecursion);
+
             RequestClientReset(true);
         }
 
         ~ApiClient()
         {
+            _lock?.Dispose();
+
             Client?.Dispose();
         }
 
@@ -105,22 +110,18 @@ public Func<HttpMessageHandler> Handler
         public ISerializer Serializer { get; set; }
 
         /// <summary>
-        /// <see cref="HttpClient"/> used by these requests. This is used by the library and as such, should **not** be disposed in any way
+        /// <see cref="HttpClient"/> used by these requests.
+        /// This is used by the library and as such, should **not** be disposed in any way
         /// </summary>
         protected HttpClient Client { get; private set; }
 
-        /// <summary>
-        /// Time, in milliseconds to wait to modify a <see cref="HttpClient"/> before failing the request
-        /// </summary>
-        protected virtual int AdjustmentTimeout => 200;
-
         #endregion
 
         #region Private Vars
 
+        private readonly ReaderWriterLockSlim _lock;
+        private long _clientAdjustmentRequestSignal;
         private Func<HttpMessageHandler> _handler;
-        private int _clientAdjustmentSignal;
-        private long _clientAdjustmentRequestSignal, _currentRequests;
 
         #endregion
 
@@ -134,50 +135,41 @@ protected HttpClient GetClient()
             // return current client if there are no changes
             var resetLevel = Interlocked.Read(ref _clientAdjustmentRequestSignal);
 
-            if (resetLevel == 0)
-            {
-                return Client;
-            }
-
-            // if we're waiting, then don't cause a crash from getting the wrong client - just wait.
-            while (Interlocked.CompareExchange(ref _clientAdjustmentSignal, 1, 0) == 1)
+            if (resetLevel > 0)
             {
-                Timeout();
-            }
+                // block all reads and let all current requests finish
+                _lock.EnterWriteLock();
 
-            try
-            {
-                // wait for all ongoing requests to end
-                while (_currentRequests > 0)
+                try
                 {
-                    Timeout();
-                }
+                    // only reset the client if the handler has changed (signal = 2)
+                    var resetClient = resetLevel == 2;
 
-                // only reset the client if the handler has changed (signal = 2)
-                var resetClient = resetLevel == 2;
+                    if (resetClient)
+                    {
+                        var handler = CreateHandler();
 
-                if (resetClient)
-                {
-                    var handler = CreateHandler();
-
-                    Client?.Dispose();
-                    Client = handler != null ? new HttpClient(handler, true) : new HttpClient();
-                }
+                        Client?.Dispose();
+                        Client = handler != null ? new HttpClient(handler, true) : new HttpClient();
+                    }
 
-                // apply new headers
-                Headers.ApplyTo(Client);
+                    // apply new headers
+                    Headers.ApplyTo(Client);
 
-                // allow the conumer to change the client
-                SetupClient(Client, resetClient);
+                    // allow the consumer to change the client
+                    SetupClient(Client, resetClient);
 
-                // reset the state
-                Interlocked.Exchange(ref _clientAdjustmentRequestSignal, 0);
-                return Client;
-            }
-            finally
-            {
-                Interlocked.Exchange(ref _clientAdjustmentSignal, 0);
+                    // reset the state
+                    Interlocked.Exchange(ref _clientAdjustmentRequestSignal, 0);
+                }
+                finally
+                {
+                    _lock.ExitWriteLock();
+                }
             }
+
+            _lock.EnterReadLock();
+            return Client;
         }
 
         #endregion
@@ -299,9 +291,7 @@ HttpResponseMessage CopyProcess(HttpResponseMessage response)
         /// <param name="disposeResponse">Whether to dispose of the <see cref="HttpResponseMessage"/> produced after <see cref="processResult"/> has been invoked.</param>
         protected T InternalPerform<T>(HttpRequestMessage request, Func<HttpResponseMessage, T> processResult, bool disposeResponse, CancellationToken token = default)
         {
-            //get client and request (disposables)
             var client = GetClient();
-            Interlocked.Increment(ref _currentRequests);
 
             // post-modification
             SetupRequest(request);
@@ -318,15 +308,14 @@ protected T InternalPerform<T>(HttpRequestMessage request, Func<HttpResponseMess
                 response = client.SendAsync(request, HttpCompletionOption.ResponseHeadersRead, token).Result;
 #endif
 
-                //all possible exceptions from client.SendAsync() will be released here
+                // all possible exceptions from client.SendAsync() will be released here
                 return processResult.Invoke(response);
             }
             finally
             {
-                //un-bump reqs
-                Interlocked.Decrement(ref _currentRequests);
+                RequestFinished();
 
-                //dispose
+                // dispose
                 if (disposeResponse)
                 {
                     response?.Dispose();
@@ -366,6 +355,10 @@ protected virtual void ValidateRequest(ApiRequest request)
             }
         }
 
+        /// <summary>
+        /// Requests the client is reset on the next request
+        /// </summary>
+        /// <param name="fullReset">Whether to reset the <see cref="HttpClient"/> as well as the headers</param>
         public void RequestClientReset(bool fullReset)
         {
             if (fullReset)
@@ -378,6 +371,7 @@ public void RequestClientReset(bool fullReset)
             }
         }
 
-        private void Timeout() => Thread.Sleep(AdjustmentTimeout / 2);
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        protected void RequestFinished() => _lock.ExitReadLock();
     }
 }
diff --git a/DragonFruit.Common.Data/Headers/HeaderCollection.cs b/DragonFruit.Common.Data/Headers/HeaderCollection.cs
index ea8c74f..53d50f8 100644
--- a/DragonFruit.Common.Data/Headers/HeaderCollection.cs
+++ b/DragonFruit.Common.Data/Headers/HeaderCollection.cs
@@ -9,11 +9,12 @@ namespace DragonFruit.Common.Data.Headers
 {
     public class HeaderCollection
     {
-        private readonly ConcurrentDictionary<string, string> _values = new ConcurrentDictionary<string, string>();
+        private readonly ConcurrentDictionary<string, string> _values;
         private readonly ApiClient _client;
 
         public HeaderCollection(ApiClient client)
         {
+            _values = new ConcurrentDictionary<string, string>();
             _client = client;
         }