From 1e02f27c4931257274858aa8670882c27b7f96cf Mon Sep 17 00:00:00 2001
From: Rutherther <fandabohacek@gmail.com>
Date: Thu, 19 Jan 2023 19:40:40 +0100
Subject: [PATCH] feat(core): avoid deadlock in contractor

---
 Core/NosSmooth.Core/Contracts/Contractor.cs | 93 ++++++++++++---------
 Core/NosSmooth.Core/NosSmooth.Core.csproj   |  8 +-
 2 files changed, 58 insertions(+), 43 deletions(-)

diff --git a/Core/NosSmooth.Core/Contracts/Contractor.cs b/Core/NosSmooth.Core/Contracts/Contractor.cs
index 028bcff..014dfee 100644
--- a/Core/NosSmooth.Core/Contracts/Contractor.cs
+++ b/Core/NosSmooth.Core/Contracts/Contractor.cs
@@ -27,14 +27,14 @@ public class Contractor : IEnumerable<IContract>
     public static readonly TimeSpan Timeout = new TimeSpan(0, 5, 0);
 
     private readonly List<ContractInfo> _contracts;
-    private readonly SemaphoreSlim _semaphore;
+    private readonly ReaderWriterLockSlim _semaphore;
 
     /// <summary>
     /// Initializes a new instance of the <see cref="Contractor"/> class.
     /// </summary>
     public Contractor()
     {
-        _semaphore = new SemaphoreSlim(1, 1);
+        _semaphore = new ReaderWriterLockSlim(LockRecursionPolicy.NoRecursion);
         _contracts = new List<ContractInfo>();
     }
 
@@ -46,12 +46,12 @@ public class Contractor : IEnumerable<IContract>
     {
         try
         {
-            _semaphore.Wait();
+            _semaphore.EnterWriteLock();
             _contracts.Add(new ContractInfo(contract, DateTime.Now));
         }
         finally
         {
-            _semaphore.Release();
+            _semaphore.ExitWriteLock();
         }
     }
 
@@ -63,12 +63,12 @@ public class Contractor : IEnumerable<IContract>
     {
         try
         {
-            _semaphore.Wait();
-            _contracts.RemoveAll(ci => ci.contract == contract);
+            _semaphore.EnterWriteLock();
+            _contracts.RemoveAll(x => x.contract == contract);
         }
         finally
         {
-            _semaphore.Release();
+            _semaphore.ExitWriteLock();
         }
     }
 
@@ -91,51 +91,66 @@ public class Contractor : IEnumerable<IContract>
     {
         var errors = new List<IResult>();
         var toRemove = new List<ContractInfo>();
+        ContractInfo[] contracts;
+
         try
         {
-            await _semaphore.WaitAsync(ct);
-            foreach (var info in _contracts)
+            _semaphore.EnterReadLock();
+            contracts = _contracts.ToArray();
+        }
+        finally
+        {
+            _semaphore.ExitReadLock();
+        }
+
+        foreach (var info in contracts)
+        {
+            if (DateTime.Now - info.addedAt > Timeout)
             {
-                if (DateTime.Now - info.addedAt > Timeout)
-                {
-                    errors.Add
+                errors.Add
+                (
+                    (Result)new GenericError
                     (
-                        (Result)new GenericError
-                        (
-                            $"A contract {info.contract} has been registered for too long and was unregistered automatically."
-                        )
-                    );
-                    continue;
-                }
-
-                var result = await info.contract.Update(data);
-                if (!result.IsDefined(out var response))
-                {
-                    errors.Add(result);
-                }
-
-                if (response == ContractUpdateResponse.InterestedAndUnregister)
-                {
-                    toRemove.Add(info);
-                }
+                        $"A contract {info.contract} has been registered for too long and was unregistered automatically."
+                    )
+                );
+                continue;
             }
 
-            foreach (var contract in toRemove)
+            var result = await info.contract.Update(data, ct);
+            if (!result.IsDefined(out var response))
             {
-                _contracts.Remove(contract);
+                errors.Add(result);
             }
 
-            return errors.Count switch
+            if (response == ContractUpdateResponse.InterestedAndUnregister)
             {
-                0 => Result.FromSuccess(),
-                1 => (Result)errors[0],
-                _ => new AggregateError(errors)
-            };
+                toRemove.Add(info);
+            }
         }
-        finally
+
+        if (toRemove.Count > 0)
         {
-            _semaphore.Release();
+            try
+            {
+                _semaphore.EnterWriteLock();
+                foreach (var contract in toRemove)
+                {
+                    _contracts.Remove(contract);
+                }
+            }
+            finally
+            {
+                _semaphore.ExitWriteLock();
+            }
         }
+
+        return errors.Count switch
+        {
+            0 => Result.FromSuccess(),
+            1 => (Result)errors[0],
+            _ => new AggregateError(errors)
+        };
     }
 
     /// <inheritdoc />
diff --git a/Core/NosSmooth.Core/NosSmooth.Core.csproj b/Core/NosSmooth.Core/NosSmooth.Core.csproj
index a94bc49..af4c042 100644
--- a/Core/NosSmooth.Core/NosSmooth.Core.csproj
+++ b/Core/NosSmooth.Core/NosSmooth.Core.csproj
@@ -8,10 +8,10 @@
         <Description>NosSmooth Core library allowing implementing nostale client, handling packets and commands.</Description>
         <RepositoryUrl>https://github.com/Rutherther/NosSmooth/</RepositoryUrl>
         <PackageLicenseExpression>MIT</PackageLicenseExpression>
-        <PackageVersion>3.1.4</PackageVersion>
-        <PackageReleaseNotes>Update dependencies.</PackageReleaseNotes>
-        <AssemblyVersion>3.1.4</AssemblyVersion>
-        <FileVersion>3.1.4</FileVersion>
+        <PackageVersion>3.1.5</PackageVersion>
+        <PackageReleaseNotes>Avoid contractor dead lock, make contractor more performant.</PackageReleaseNotes>
+        <AssemblyVersion>3.1.5</AssemblyVersion>
+        <FileVersion>3.1.5</FileVersion>
     </PropertyGroup>
 
     <ItemGroup>
-- 
2.49.0