From c0172963d5d92fb41c79e02eeeb8ebf42eee46e7 Mon Sep 17 00:00:00 2001 From: Vinzenz Schroeter Date: Sun, 28 Apr 2024 15:34:32 +0200 Subject: [PATCH] potential fix for locking issues --- tanks-backend/TanksServer/Endpoints.cs | 4 +- .../TanksServer/GameLogic/CollectPowerUp.cs | 4 +- .../TanksServer/GameLogic/CollideBullets.cs | 4 +- .../TanksServer/GameLogic/ITickStep.cs | 2 +- .../TanksServer/GameLogic/MoveBullets.cs | 4 +- .../TanksServer/GameLogic/MoveTanks.cs | 4 +- .../TanksServer/GameLogic/RotateTanks.cs | 4 +- .../TanksServer/GameLogic/ShootFromTanks.cs | 4 +- .../TanksServer/GameLogic/SpawnPowerUp.cs | 8 +- .../TanksServer/GameLogic/TankSpawnQueue.cs | 6 +- .../Graphics/GeneratePixelsTickStep.cs | 2 +- .../TanksServer/Graphics/IFrameConsumer.cs | 2 +- .../Interactivity/ClientScreenServer.cs | 2 +- .../ClientScreenServerConnection.cs | 119 +++++++----------- .../Interactivity/ControlsServerConnection.cs | 2 +- .../Interactivity/PlayerInfoConnection.cs | 25 ++-- .../TanksServer/Interactivity/PlayerServer.cs | 6 +- .../SendToServicePointDisplay.cs | 2 +- .../Interactivity/WebsocketServer.cs | 27 ++-- .../WebsocketServerConnection.cs | 22 +++- 20 files changed, 112 insertions(+), 141 deletions(-) diff --git a/tanks-backend/TanksServer/Endpoints.cs b/tanks-backend/TanksServer/Endpoints.cs index 485fe42..1267084 100644 --- a/tanks-backend/TanksServer/Endpoints.cs +++ b/tanks-backend/TanksServer/Endpoints.cs @@ -80,8 +80,6 @@ internal sealed class Endpoints( if (name.Length > 12) return TypedResults.BadRequest("name too long"); var player = playerService.GetOrAdd(name); - return player != null - ? TypedResults.Ok(player.Name) - : TypedResults.Unauthorized(); + return TypedResults.Ok(player.Name); } } diff --git a/tanks-backend/TanksServer/GameLogic/CollectPowerUp.cs b/tanks-backend/TanksServer/GameLogic/CollectPowerUp.cs index 04afe67..c5af079 100644 --- a/tanks-backend/TanksServer/GameLogic/CollectPowerUp.cs +++ b/tanks-backend/TanksServer/GameLogic/CollectPowerUp.cs @@ -4,10 +4,10 @@ internal sealed class CollectPowerUp( MapEntityManager entityManager ) : ITickStep { - public Task TickAsync(TimeSpan delta) + public ValueTask TickAsync(TimeSpan delta) { entityManager.RemoveWhere(TryCollect); - return Task.CompletedTask; + return ValueTask.CompletedTask; } private bool TryCollect(PowerUp obj) diff --git a/tanks-backend/TanksServer/GameLogic/CollideBullets.cs b/tanks-backend/TanksServer/GameLogic/CollideBullets.cs index 9e56338..a69eebb 100644 --- a/tanks-backend/TanksServer/GameLogic/CollideBullets.cs +++ b/tanks-backend/TanksServer/GameLogic/CollideBullets.cs @@ -9,12 +9,12 @@ internal sealed class CollideBullets( { private const int ExplosionRadius = 3; - public Task TickAsync(TimeSpan _) + public ValueTask TickAsync(TimeSpan _) { entityManager.RemoveWhere(BulletHitsTank); entityManager.RemoveWhere(BulletHitsWall); entityManager.RemoveWhere(BulletTimesOut); - return Task.CompletedTask; + return ValueTask.CompletedTask; } private bool BulletTimesOut(Bullet bullet) diff --git a/tanks-backend/TanksServer/GameLogic/ITickStep.cs b/tanks-backend/TanksServer/GameLogic/ITickStep.cs index f2501d7..1cf44f5 100644 --- a/tanks-backend/TanksServer/GameLogic/ITickStep.cs +++ b/tanks-backend/TanksServer/GameLogic/ITickStep.cs @@ -2,5 +2,5 @@ namespace TanksServer.GameLogic; public interface ITickStep { - Task TickAsync(TimeSpan delta); + ValueTask TickAsync(TimeSpan delta); } diff --git a/tanks-backend/TanksServer/GameLogic/MoveBullets.cs b/tanks-backend/TanksServer/GameLogic/MoveBullets.cs index c9a7f1a..63e3661 100644 --- a/tanks-backend/TanksServer/GameLogic/MoveBullets.cs +++ b/tanks-backend/TanksServer/GameLogic/MoveBullets.cs @@ -5,12 +5,12 @@ internal sealed class MoveBullets( IOptions config ) : ITickStep { - public Task TickAsync(TimeSpan delta) + public ValueTask TickAsync(TimeSpan delta) { foreach (var bullet in entityManager.Bullets) MoveBullet(bullet, delta); - return Task.CompletedTask; + return ValueTask.CompletedTask; } private void MoveBullet(Bullet bullet, TimeSpan delta) diff --git a/tanks-backend/TanksServer/GameLogic/MoveTanks.cs b/tanks-backend/TanksServer/GameLogic/MoveTanks.cs index a9e71d9..a007b03 100644 --- a/tanks-backend/TanksServer/GameLogic/MoveTanks.cs +++ b/tanks-backend/TanksServer/GameLogic/MoveTanks.cs @@ -8,12 +8,12 @@ internal sealed class MoveTanks( { private readonly GameRules _config = options.Value; - public Task TickAsync(TimeSpan delta) + public ValueTask TickAsync(TimeSpan delta) { foreach (var tank in entityManager.Tanks) tank.Moving = TryMoveTank(tank, delta); - return Task.CompletedTask; + return ValueTask.CompletedTask; } private bool TryMoveTank(Tank tank, TimeSpan delta) diff --git a/tanks-backend/TanksServer/GameLogic/RotateTanks.cs b/tanks-backend/TanksServer/GameLogic/RotateTanks.cs index f7c36bf..8fd9007 100644 --- a/tanks-backend/TanksServer/GameLogic/RotateTanks.cs +++ b/tanks-backend/TanksServer/GameLogic/RotateTanks.cs @@ -8,7 +8,7 @@ internal sealed class RotateTanks( { private readonly GameRules _config = options.Value; - public Task TickAsync(TimeSpan delta) + public ValueTask TickAsync(TimeSpan delta) { foreach (var tank in entityManager.Tanks) { @@ -30,6 +30,6 @@ internal sealed class RotateTanks( logger.LogTrace("rotated tank to {}", tank.Rotation); } - return Task.CompletedTask; + return ValueTask.CompletedTask; } } diff --git a/tanks-backend/TanksServer/GameLogic/ShootFromTanks.cs b/tanks-backend/TanksServer/GameLogic/ShootFromTanks.cs index e816d5b..e6bb65e 100644 --- a/tanks-backend/TanksServer/GameLogic/ShootFromTanks.cs +++ b/tanks-backend/TanksServer/GameLogic/ShootFromTanks.cs @@ -9,12 +9,12 @@ internal sealed class ShootFromTanks( { private readonly GameRules _config = options.Value; - public Task TickAsync(TimeSpan _) + public ValueTask TickAsync(TimeSpan _) { foreach (var tank in entityManager.Tanks.Where(t => !t.Moving)) Shoot(tank); - return Task.CompletedTask; + return ValueTask.CompletedTask; } private void Shoot(Tank tank) diff --git a/tanks-backend/TanksServer/GameLogic/SpawnPowerUp.cs b/tanks-backend/TanksServer/GameLogic/SpawnPowerUp.cs index bc362bb..f14a60e 100644 --- a/tanks-backend/TanksServer/GameLogic/SpawnPowerUp.cs +++ b/tanks-backend/TanksServer/GameLogic/SpawnPowerUp.cs @@ -8,14 +8,14 @@ internal sealed class SpawnPowerUp( private readonly double _spawnChance = options.Value.PowerUpSpawnChance; private readonly int _maxCount = options.Value.MaxPowerUpCount; - public Task TickAsync(TimeSpan delta) + public ValueTask TickAsync(TimeSpan delta) { if (entityManager.PowerUps.Count() >= _maxCount) - return Task.CompletedTask; + return ValueTask.CompletedTask; if (Random.Shared.NextDouble() > _spawnChance * delta.TotalSeconds) - return Task.CompletedTask; + return ValueTask.CompletedTask; entityManager.SpawnPowerUp(); - return Task.CompletedTask; + return ValueTask.CompletedTask; } } diff --git a/tanks-backend/TanksServer/GameLogic/TankSpawnQueue.cs b/tanks-backend/TanksServer/GameLogic/TankSpawnQueue.cs index 447cec9..4abb0ae 100644 --- a/tanks-backend/TanksServer/GameLogic/TankSpawnQueue.cs +++ b/tanks-backend/TanksServer/GameLogic/TankSpawnQueue.cs @@ -43,12 +43,12 @@ internal sealed class TankSpawnQueue( return true; } - public Task TickAsync(TimeSpan _) + public ValueTask TickAsync(TimeSpan _) { if (!TryDequeueNext(out var player)) - return Task.CompletedTask; + return ValueTask.CompletedTask; entityManager.SpawnTank(player); - return Task.CompletedTask; + return ValueTask.CompletedTask; } } diff --git a/tanks-backend/TanksServer/Graphics/GeneratePixelsTickStep.cs b/tanks-backend/TanksServer/Graphics/GeneratePixelsTickStep.cs index f09e095..b2a7f36 100644 --- a/tanks-backend/TanksServer/Graphics/GeneratePixelsTickStep.cs +++ b/tanks-backend/TanksServer/Graphics/GeneratePixelsTickStep.cs @@ -12,7 +12,7 @@ internal sealed class GeneratePixelsTickStep( private readonly List _drawSteps = drawSteps.ToList(); private readonly List _consumers = consumers.ToList(); - public async Task TickAsync(TimeSpan _) + public async ValueTask TickAsync(TimeSpan _) { PixelGrid observerPixelGrid = new(MapService.PixelsPerRow, MapService.PixelsPerColumn); diff --git a/tanks-backend/TanksServer/Graphics/IFrameConsumer.cs b/tanks-backend/TanksServer/Graphics/IFrameConsumer.cs index 5a83a86..cc6521f 100644 --- a/tanks-backend/TanksServer/Graphics/IFrameConsumer.cs +++ b/tanks-backend/TanksServer/Graphics/IFrameConsumer.cs @@ -4,5 +4,5 @@ namespace TanksServer.Graphics; internal interface IFrameConsumer { - Task OnFrameDoneAsync(GamePixelGrid gamePixelGrid, PixelGrid observerPixels); + ValueTask OnFrameDoneAsync(GamePixelGrid gamePixelGrid, PixelGrid observerPixels); } diff --git a/tanks-backend/TanksServer/Interactivity/ClientScreenServer.cs b/tanks-backend/TanksServer/Interactivity/ClientScreenServer.cs index 0915bbf..8e3b498 100644 --- a/tanks-backend/TanksServer/Interactivity/ClientScreenServer.cs +++ b/tanks-backend/TanksServer/Interactivity/ClientScreenServer.cs @@ -16,6 +16,6 @@ internal sealed class ClientScreenServer( player )); - public Task OnFrameDoneAsync(GamePixelGrid gamePixelGrid, PixelGrid observerPixels) + public ValueTask OnFrameDoneAsync(GamePixelGrid gamePixelGrid, PixelGrid observerPixels) => ParallelForEachConnectionAsync(c => c.OnGameTickAsync(observerPixels, gamePixelGrid).AsTask()); } diff --git a/tanks-backend/TanksServer/Interactivity/ClientScreenServerConnection.cs b/tanks-backend/TanksServer/Interactivity/ClientScreenServerConnection.cs index fe019f4..b963690 100644 --- a/tanks-backend/TanksServer/Interactivity/ClientScreenServerConnection.cs +++ b/tanks-backend/TanksServer/Interactivity/ClientScreenServerConnection.cs @@ -8,93 +8,66 @@ internal sealed class ClientScreenServerConnection( WebSocket webSocket, ILogger logger, string? playerName = null -) : WebsocketServerConnection(logger, new ByteChannelWebSocket(webSocket, logger, 0)), - IDisposable +) : WebsocketServerConnection(logger, new ByteChannelWebSocket(webSocket, logger, 0)) { - private readonly SemaphoreSlim _wantedFramesOnTick = new(0, 2); - private readonly SemaphoreSlim _mutex = new(1); + private bool _wantsFrameOnTick = true; - private PixelGrid? _lastSentPixels = null; - private PixelGrid? _nextPixels = null; + private PixelGrid? _lastSentPixels; + private PixelGrid? _nextPixels; private readonly PlayerScreenData? _nextPlayerData = playerName != null ? new PlayerScreenData(logger) : null; - protected override async ValueTask HandleMessageAsync(Memory _) + protected override async ValueTask HandleMessageLockedAsync(Memory _) { - await _mutex.WaitAsync(); + if (_nextPixels == null) + { + _wantsFrameOnTick = true; + return; + } + + await SendNowAsync(); + } + + public ValueTask OnGameTickAsync(PixelGrid pixels, GamePixelGrid gamePixelGrid) => LockedAsync(async () => + { + if (pixels == _lastSentPixels) + return; + + if (_nextPlayerData != null) + { + _nextPlayerData.Clear(); + foreach (var gamePixel in gamePixelGrid) + { + if (!gamePixel.EntityType.HasValue) + continue; + _nextPlayerData.Add(gamePixel.EntityType.Value, gamePixel.BelongsTo?.Name == playerName); + } + } + + _nextPixels = pixels; + if (_wantsFrameOnTick) + _ = await SendNowAsync(); + }); + + private async ValueTask SendNowAsync() + { + var pixels = _nextPixels + ?? throw new InvalidOperationException("next pixels not set"); + try { - if (_nextPixels == null) - { - _wantedFramesOnTick.Release(); - return; - } + await Socket.SendBinaryAsync(pixels.Data, _nextPlayerData == null); + if (_nextPlayerData != null) + await Socket.SendBinaryAsync(_nextPlayerData.GetPacket()); _lastSentPixels = _nextPixels; _nextPixels = null; - await SendNowAsync(_lastSentPixels); - } - catch (SemaphoreFullException) - { - logger.LogWarning("ignoring request for more frames"); - } - finally - { - _mutex.Release(); - } - } - - public async ValueTask OnGameTickAsync(PixelGrid pixels, GamePixelGrid gamePixelGrid) - { - await _mutex.WaitAsync(); - try - { - if (pixels == _lastSentPixels) - return; - - if (_nextPlayerData != null) - { - _nextPlayerData.Clear(); - foreach (var gamePixel in gamePixelGrid) - { - if (!gamePixel.EntityType.HasValue) - continue; - _nextPlayerData.Add(gamePixel.EntityType.Value, gamePixel.BelongsTo?.Name == playerName); - } - } - - var sendImmediately = await _wantedFramesOnTick.WaitAsync(TimeSpan.Zero); - if (sendImmediately) - { - await SendNowAsync(pixels); - return; - } - - _wantedFramesOnTick.Release(); - _nextPixels = pixels; - } - finally - { - _mutex.Release(); - } - } - - private async ValueTask SendNowAsync(PixelGrid pixels) - { - Logger.LogTrace("sending"); - try - { - Logger.LogTrace("sending {} bytes of pixel data", pixels.Data.Length); - await Socket.SendBinaryAsync(pixels.Data, _nextPlayerData == null); - if (_nextPlayerData != null) - { - await Socket.SendBinaryAsync(_nextPlayerData.GetPacket()); - } + _wantsFrameOnTick = false; + return true; } catch (WebSocketException ex) { Logger.LogWarning(ex, "send failed"); + return false; } } - - public void Dispose() => _wantedFramesOnTick.Dispose(); } diff --git a/tanks-backend/TanksServer/Interactivity/ControlsServerConnection.cs b/tanks-backend/TanksServer/Interactivity/ControlsServerConnection.cs index 35f57f4..ac8e9b6 100644 --- a/tanks-backend/TanksServer/Interactivity/ControlsServerConnection.cs +++ b/tanks-backend/TanksServer/Interactivity/ControlsServerConnection.cs @@ -23,7 +23,7 @@ internal sealed class ControlsServerConnection( Shoot = 0x05 } - protected override ValueTask HandleMessageAsync(Memory buffer) + protected override ValueTask HandleMessageLockedAsync(Memory buffer) { var type = (MessageType)buffer.Span[0]; var control = (InputType)buffer.Span[1]; diff --git a/tanks-backend/TanksServer/Interactivity/PlayerInfoConnection.cs b/tanks-backend/TanksServer/Interactivity/PlayerInfoConnection.cs index d3109b7..74b39e4 100644 --- a/tanks-backend/TanksServer/Interactivity/PlayerInfoConnection.cs +++ b/tanks-backend/TanksServer/Interactivity/PlayerInfoConnection.cs @@ -10,19 +10,19 @@ internal sealed class PlayerInfoConnection( ILogger logger, WebSocket rawSocket, MapEntityManager entityManager -) : WebsocketServerConnection(logger, new ByteChannelWebSocket(rawSocket, logger, 0)), IDisposable +) : WebsocketServerConnection(logger, new ByteChannelWebSocket(rawSocket, logger, 0)) { - private readonly SemaphoreSlim _wantedFrames = new(1); private readonly AppSerializerContext _context = new(new JsonSerializerOptions(JsonSerializerDefaults.Web)); + private bool _wantsInfoOnTick; private byte[] _lastMessage = []; - protected override ValueTask HandleMessageAsync(Memory buffer) + protected override ValueTask HandleMessageLockedAsync(Memory buffer) { var response = GetMessageToSend(); if (response == null) { Logger.LogTrace("cannot respond directly, increasing wanted frames"); - _wantedFrames.Release(); + _wantsInfoOnTick = true; return ValueTask.CompletedTask; } @@ -30,21 +30,18 @@ internal sealed class PlayerInfoConnection( return Socket.SendTextAsync(response); } - public async Task OnGameTickAsync() + public ValueTask OnGameTickAsync() => LockedAsync(() => { - if (!await _wantedFrames.WaitAsync(TimeSpan.Zero)) - return; + if (!_wantsInfoOnTick) + return ValueTask.CompletedTask; var response = GetMessageToSend(); if (response == null) - { - _wantedFrames.Release(); - return; - } + return ValueTask.CompletedTask; Logger.LogTrace("responding indirectly"); - await Socket.SendTextAsync(response); - } + return Socket.SendTextAsync(response); + }); private byte[]? GetMessageToSend() { @@ -77,6 +74,4 @@ internal sealed class PlayerInfoConnection( str.Append(']'); return str.ToString(); } - - public void Dispose() => _wantedFrames.Dispose(); } diff --git a/tanks-backend/TanksServer/Interactivity/PlayerServer.cs b/tanks-backend/TanksServer/Interactivity/PlayerServer.cs index 2dfa9e1..200c77e 100644 --- a/tanks-backend/TanksServer/Interactivity/PlayerServer.cs +++ b/tanks-backend/TanksServer/Interactivity/PlayerServer.cs @@ -14,7 +14,7 @@ internal sealed class PlayerServer( private readonly Dictionary _players = []; private readonly SemaphoreSlim _mutex = new(1, 1); - public Player? GetOrAdd(string name) + public Player GetOrAdd(string name) { _mutex.Wait(); try @@ -67,6 +67,6 @@ internal sealed class PlayerServer( public Task HandleClientAsync(WebSocket webSocket, Player player) => HandleClientAsync(new PlayerInfoConnection(player, connectionLogger, webSocket, entityManager)); - public Task TickAsync(TimeSpan delta) - => ParallelForEachConnectionAsync(connection => connection.OnGameTickAsync()); + public ValueTask TickAsync(TimeSpan delta) + => ParallelForEachConnectionAsync(connection => connection.OnGameTickAsync().AsTask()); } diff --git a/tanks-backend/TanksServer/Interactivity/SendToServicePointDisplay.cs b/tanks-backend/TanksServer/Interactivity/SendToServicePointDisplay.cs index 614619c..b73f2ff 100644 --- a/tanks-backend/TanksServer/Interactivity/SendToServicePointDisplay.cs +++ b/tanks-backend/TanksServer/Interactivity/SendToServicePointDisplay.cs @@ -48,7 +48,7 @@ internal sealed class SendToServicePointDisplay : IFrameConsumer }; } - public async Task OnFrameDoneAsync(GamePixelGrid gamePixelGrid, PixelGrid observerPixels) + public async ValueTask OnFrameDoneAsync(GamePixelGrid gamePixelGrid, PixelGrid observerPixels) { if (DateTime.Now < _nextFrameAfter) return; diff --git a/tanks-backend/TanksServer/Interactivity/WebsocketServer.cs b/tanks-backend/TanksServer/Interactivity/WebsocketServer.cs index d5678a2..f8c6719 100644 --- a/tanks-backend/TanksServer/Interactivity/WebsocketServer.cs +++ b/tanks-backend/TanksServer/Interactivity/WebsocketServer.cs @@ -14,7 +14,7 @@ internal abstract class WebsocketServer( public async Task StoppingAsync(CancellationToken cancellationToken) { logger.LogInformation("closing connections"); - await Locked(async () => + await LockedAsync(async () => { _closing = true; await Task.WhenAll(_connections.Select(c => c.CloseAsync())); @@ -22,35 +22,24 @@ internal abstract class WebsocketServer( logger.LogInformation("closed connections"); } - protected Task ParallelForEachConnectionAsync(Func body) - { - _mutex.Wait(); - try - { - return Task.WhenAll(_connections.Select(body)); - } - finally - { - _mutex.Release(); - } - } + protected ValueTask ParallelForEachConnectionAsync(Func body) => + LockedAsync(async () => await Task.WhenAll(_connections.Select(body)), CancellationToken.None); - private Task AddConnectionAsync(T connection) => Locked(() => + private ValueTask AddConnectionAsync(T connection) => LockedAsync(async () => { if (_closing) { logger.LogWarning("refusing connection because server is shutting down"); - return connection.CloseAsync(); + await connection.CloseAsync(); } _connections.Add(connection); - return Task.CompletedTask; }, CancellationToken.None); - private Task RemoveConnectionAsync(T connection) => Locked(() => + private ValueTask RemoveConnectionAsync(T connection) => LockedAsync(() => { _connections.Remove(connection); - return Task.CompletedTask; + return ValueTask.CompletedTask; }, CancellationToken.None); protected async Task HandleClientAsync(T connection) @@ -60,7 +49,7 @@ internal abstract class WebsocketServer( await RemoveConnectionAsync(connection); } - private async Task Locked(Func action, CancellationToken cancellationToken) + private async ValueTask LockedAsync(Func action, CancellationToken cancellationToken) { await _mutex.WaitAsync(cancellationToken); try diff --git a/tanks-backend/TanksServer/Interactivity/WebsocketServerConnection.cs b/tanks-backend/TanksServer/Interactivity/WebsocketServerConnection.cs index 629c44b..c8feccc 100644 --- a/tanks-backend/TanksServer/Interactivity/WebsocketServerConnection.cs +++ b/tanks-backend/TanksServer/Interactivity/WebsocketServerConnection.cs @@ -3,8 +3,9 @@ namespace TanksServer.Interactivity; internal abstract class WebsocketServerConnection( ILogger logger, ByteChannelWebSocket socket -) +) : IDisposable { + private readonly SemaphoreSlim _mutex = new(1); protected readonly ByteChannelWebSocket Socket = socket; protected readonly ILogger Logger = logger; @@ -17,9 +18,24 @@ internal abstract class WebsocketServerConnection( public async Task ReceiveAsync() { await foreach (var buffer in Socket.ReadAllAsync()) - await HandleMessageAsync(buffer); + await LockedAsync(() => HandleMessageLockedAsync(buffer)); Logger.LogTrace("done receiving"); } - protected abstract ValueTask HandleMessageAsync(Memory buffer); + protected abstract ValueTask HandleMessageLockedAsync(Memory buffer); + + protected async ValueTask LockedAsync(Func action) + { + await _mutex.WaitAsync(); + try + { + await action(); + } + finally + { + _mutex.Release(); + } + } + + public void Dispose() => _mutex.Dispose(); }