Add a lock on most interactions with game objects

We see rare problems where multiple messages on the same game arriving at
the same game cause issues. This commit adds a coarse-grained lock on
all interactions with a game by adding a binary semaphore.

Because manually adding try{} finally{} blocks seems verbose, I added a
RAII-style LockGuard that locks the semaphore on creation and drops it
when disposed.

src/Impostor.Api/Games/IGame.cs

@@ -1,6 +1,7 @@
 using System.Collections.Generic;
 using System.Net;
 using System.Threading.Tasks;
+using Impostor.Api.Events;
 using Impostor.Api.Innersloth;
 using Impostor.Api.Innersloth.GameOptions;
 using Impostor.Api.Net;
@@ -52,6 +53,11 @@ namespace Impostor.Api.Games
         T? FindObjectByNetId<T>(uint netId)
             where T : IInnerNetObject;
 
+        /// <summary>Lock an IGame to prevent concurrent modification.</summary>
+        /// <returns>A <see cref="LockGuard" /> that should be awaited before and disposed after modifying the game.</returns>
+        /// <remarks>This method should be called when modifying a game outside of an <see cref="IGameEvent" /> handler.</remarks>
+        Task<LockGuard> LockAsync();
+
         /// <summary>
         ///     Adds an <see cref="IPAddress" /> to the ban list of this game.
         ///     Prevents all future joins from this <see cref="IPAddress" />.

src/Impostor.Api/Net/LockGuard.cs

@@ -0,0 +1,61 @@
+using System;
+using System.Threading;
+using System.Threading.Tasks;
+
+namespace Impostor.Api.Net
+{
+    /// <summary>RAII-style lock guard for use in Impostor.</summary>
+    /// <remarks>
+    /// To lock a semaphore, use the LockAsync method and await the result.
+    /// To unlock the semaphore, dispose of the LockGuard.
+    /// </remarks>
+    public sealed class LockGuard : IDisposable
+    {
+        private readonly SemaphoreSlim _semaphore;
+
+        private bool _disposedValue;
+
+        private LockGuard(SemaphoreSlim semaphore)
+        {
+            _semaphore = semaphore;
+        }
+
+        private void Dispose(bool disposing)
+        {
+            if (!_disposedValue)
+            {
+                if (disposing)
+                {
+                    _semaphore.Release();
+                }
+
+                _disposedValue = true;
+            }
+        }
+
+        /// <summary>Create a RAII-style lock guard for an instance of <see cref="SemaphoreSlim"/>.</summary>
+        /// <param name="semaphore">The semaphore to Wait and Release on.</param>
+        /// <param name="timeout">The amount of milliseconds to wait for the lock becoming available.</param>
+        /// <exception cref="ImpostorException">When the timeout expires.</exception>
+        /// <returns>The lock guard if the semaphore could be locked succesfully.</returns>
+        public static async Task<LockGuard> LockAsync(SemaphoreSlim semaphore, int timeout = 1000)
+        {
+            var success = await semaphore.WaitAsync(timeout);
+            if (success)
+            {
+                return new LockGuard(semaphore);
+            }
+            else
+            {
+                throw new ImpostorException("Could not lock semaphore");
+            }
+        }
+
+        /// <inheritdoc/>
+        public void Dispose()
+        {
+            Dispose(disposing: true);
+            GC.SuppressFinalize(this);
+        }
+    }
+}

src/Impostor.Server/Net/State/Game.Data.cs

@@ -63,6 +63,8 @@ namespace Impostor.Server.Net.State
 
         public async ValueTask<bool> HandleGameDataAsync(IMessageReader parent, ClientPlayer sender, bool toPlayer)
         {
+            using var guard = await LockAsync();
+
             // Find target player.
             ClientPlayer? target = null;
 

src/Impostor.Server/Net/State/Game.Incoming.cs

@@ -1,5 +1,3 @@
-using System;
-using System.Threading;
 using System.Threading.Tasks;
 using Impostor.Api.Games;
 using Impostor.Api.Innersloth;
@@ -13,10 +11,9 @@ namespace Impostor.Server.Net.State
 {
     internal partial class Game
     {
-        private readonly SemaphoreSlim _clientAddLock = new SemaphoreSlim(1, 1);
-
         public async ValueTask HandleStartGame(IMessageReader message)
         {
+            using var guard = await LockAsync();
             GameState = GameStates.Starting;
 
             using var packet = MessageWriter.Get(MessageType.Reliable);
@@ -28,6 +25,7 @@ namespace Impostor.Server.Net.State
 
         public async ValueTask HandleEndGame(IMessageReader message, GameOverReason gameOverReason)
         {
+            using var guard = await LockAsync();
             GameState = GameStates.Ended;
 
             // Broadcast end of the game.
@@ -48,6 +46,7 @@ namespace Impostor.Server.Net.State
 
         public async ValueTask HandleAlterGame(IMessageReader message, IClientPlayer sender, bool isPublic)
         {
+            using var guard = await LockAsync();
             IsPublic = isPublic;
 
             using var packet = MessageWriter.Get(MessageType.Reliable);
@@ -74,6 +73,7 @@ namespace Impostor.Server.Net.State
 
         public async ValueTask HandleKickPlayer(int playerId, bool isBan)
         {
+            using var guard = await LockAsync();
             _logger.LogInformation("{0} - Player {1} has left.", Code, playerId);
 
             using var message = MessageWriter.Get(MessageType.Reliable);
@@ -96,26 +96,8 @@ namespace Impostor.Server.Net.State
 
         public async ValueTask<GameJoinResult> AddClientAsync(ClientBase client)
         {
-            var hasLock = false;
-
-            try
-            {
-                hasLock = await _clientAddLock.WaitAsync(TimeSpan.FromMinutes(1));
-
-                if (hasLock)
-                {
-                    return await AddClientSafeAsync(client);
-                }
-            }
-            finally
-            {
-                if (hasLock)
-                {
-                    _clientAddLock.Release();
-                }
-            }
-
-            return GameJoinResult.FromError(GameJoinError.InvalidClient);
+            using var guard = await LockAsync();
+            return await AddClientSafeAsync(client);
         }
 
         private async ValueTask HandleJoinGameNew(ClientPlayer sender, bool isNew)

src/Impostor.Server/Net/State/Game.cs

@@ -4,6 +4,7 @@ using System.Collections.Generic;
 using System.Diagnostics.CodeAnalysis;
 using System.Linq;
 using System.Net;
+using System.Threading;
 using System.Threading.Tasks;
 using Impostor.Api.Config;
 using Impostor.Api.Events.Managers;
@@ -32,6 +33,7 @@ namespace Impostor.Server.Net.State
         private readonly ICompatibilityManager _compatibilityManager;
         private readonly CompatibilityConfig _compatibilityConfig;
         private readonly TimeoutConfig _timeoutConfig;
+        private readonly SemaphoreSlim _lock;
 
         public Game(
             ILogger<Game> logger,
@@ -66,6 +68,7 @@ namespace Impostor.Server.Net.State
             _compatibilityConfig = compatibilityConfig.Value;
             _timeoutConfig = timeoutConfig.Value;
             Items = new ConcurrentDictionary<object, object>();
+            _lock = new SemaphoreSlim(1, 1);
         }
 
         public IPEndPoint PublicIp { get; }
@@ -113,6 +116,11 @@ namespace Impostor.Server.Net.State
             return _players.TryGetValue(clientId, out var clientPlayer) ? clientPlayer : null;
         }
 
+        public async Task<LockGuard> LockAsync()
+        {
+            return await LockGuard.LockAsync(_lock);
+        }
+
         internal async ValueTask StartedAsync()
         {
             if (GameState == GameStates.Starting)