From d91ab5cfe881be2982bec892f1bb6bb78bcaadcd Mon Sep 17 00:00:00 2001 From: miniduikboot Date: Thu, 16 Nov 2023 17:35:40 +0100 Subject: [PATCH] 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 | 6 ++ src/Impostor.Api/Net/LockGuard.cs | 61 +++++++++++++++++++ src/Impostor.Server/Net/State/Game.Data.cs | 2 + .../Net/State/Game.Incoming.cs | 30 ++------- src/Impostor.Server/Net/State/Game.cs | 8 +++ 5 files changed, 83 insertions(+), 24 deletions(-) create mode 100644 src/Impostor.Api/Net/LockGuard.cs diff --git a/src/Impostor.Api/Games/IGame.cs b/src/Impostor.Api/Games/IGame.cs index 05d6015b..4aabe696 100644 --- a/src/Impostor.Api/Games/IGame.cs +++ b/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(uint netId) where T : IInnerNetObject; + /// Lock an IGame to prevent concurrent modification. + /// A that should be awaited before and disposed after modifying the game. + /// This method should be called when modifying a game outside of an handler. + Task LockAsync(); + /// /// Adds an to the ban list of this game. /// Prevents all future joins from this . diff --git a/src/Impostor.Api/Net/LockGuard.cs b/src/Impostor.Api/Net/LockGuard.cs new file mode 100644 index 00000000..6590853a --- /dev/null +++ b/src/Impostor.Api/Net/LockGuard.cs @@ -0,0 +1,61 @@ +using System; +using System.Threading; +using System.Threading.Tasks; + +namespace Impostor.Api.Net +{ + /// RAII-style lock guard for use in Impostor. + /// + /// To lock a semaphore, use the LockAsync method and await the result. + /// To unlock the semaphore, dispose of the LockGuard. + /// + 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; + } + } + + /// Create a RAII-style lock guard for an instance of . + /// The semaphore to Wait and Release on. + /// The amount of milliseconds to wait for the lock becoming available. + /// When the timeout expires. + /// The lock guard if the semaphore could be locked succesfully. + public static async Task 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"); + } + } + + /// + public void Dispose() + { + Dispose(disposing: true); + GC.SuppressFinalize(this); + } + } +} diff --git a/src/Impostor.Server/Net/State/Game.Data.cs b/src/Impostor.Server/Net/State/Game.Data.cs index 2761893d..942bac6d 100644 --- a/src/Impostor.Server/Net/State/Game.Data.cs +++ b/src/Impostor.Server/Net/State/Game.Data.cs @@ -63,6 +63,8 @@ namespace Impostor.Server.Net.State public async ValueTask HandleGameDataAsync(IMessageReader parent, ClientPlayer sender, bool toPlayer) { + using var guard = await LockAsync(); + // Find target player. ClientPlayer? target = null; diff --git a/src/Impostor.Server/Net/State/Game.Incoming.cs b/src/Impostor.Server/Net/State/Game.Incoming.cs index 3d637e4d..a2d086ee 100644 --- a/src/Impostor.Server/Net/State/Game.Incoming.cs +++ b/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 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) diff --git a/src/Impostor.Server/Net/State/Game.cs b/src/Impostor.Server/Net/State/Game.cs index 918294a7..6a8849ae 100644 --- a/src/Impostor.Server/Net/State/Game.cs +++ b/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 logger, @@ -66,6 +68,7 @@ namespace Impostor.Server.Net.State _compatibilityConfig = compatibilityConfig.Value; _timeoutConfig = timeoutConfig.Value; Items = new ConcurrentDictionary(); + _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 LockAsync() + { + return await LockGuard.LockAsync(_lock); + } + internal async ValueTask StartedAsync() { if (GameState == GameStates.Starting)