Improve state management and concurrency

Dan Oak · Dan Oak · commit a29df9ae4bba · 2026-03-07T10:14:48.000Z
- Fix WorldHostedService to use async void with proper error handling
- Replace ConcurrentBag with ConcurrentDictionary for Players and NPCs (O(1) lookups)
- Make WorldState fields private with thread-safe accessors and helper methods
- Add player disconnect cleanup for trade/warp sessions
- Update all packet handlers to use .Values for dictionary collections
diff --git a/src/Acorn/Net/NewConnectionHostedService.cs b/src/Acorn/Net/NewConnectionHostedService.cs
@@ -10,6 +10,7 @@
 using Microsoft.Extensions.Hosting;
 using Microsoft.Extensions.Logging;
 using Microsoft.Extensions.Options;
+using Moffat.EndlessOnline.SDK.Protocol.Net.Server;
 
 namespace Acorn.Net;
 
@@ -117,7 +118,7 @@ await HandleWebSocketConnection(ws.Result, cancellationToken),
                     var playerState = playerStateFactory.CreatePlayerState(communicator, sessionId,
                         async player => await OnClientDisposed(player, sessionId));
 
-                    var added = worldState.Players.TryAdd(sessionId, playerState);
+                    var added = worldState.TryAddPlayer(sessionId, playerState);
                     logger.LogInformation("Connection accepted. {PlayersConnected} players connected",
                         worldState.Players.Count);
                     UpdateConnectedCount();
@@ -168,13 +169,33 @@ private void UpdateConnectedCount()
 
     private async Task OnClientDisposed(PlayerState player, int sessionId)
     {
+        // Cancel any pending warp
+        player.WarpSession = null;
+
+        // Cancel any pending trade
+        if (player.TradeSession != null)
+        {
+            var partner = player.CurrentMap?.Players.Values.FirstOrDefault(p => 
+                p.SessionId == player.TradeSession?.PartnerId);
+            
+            if (partner != null)
+            {
+                partner.TradeSession = null;
+                partner.PendingTradeRequestFromPlayerId = null;
+                await partner.Send(new TradeCloseServerPacket());
+            }
+            
+            player.TradeSession = null;
+            player.PendingTradeRequestFromPlayerId = null;
+        }
+
         if (player.Character is not null && player.CurrentMap is not null)
         {
             await player.CurrentMap.NotifyLeave(player);
             await characterRepository.UpdateAsync(characterMapper.ToDatabase(player.Character));
         }
 
-        worldState.Players.TryRemove(sessionId, out _);
+        worldState.TryRemovePlayer(sessionId, out _);
         logger.LogInformation("Player disconnected");
         UpdateConnectedCount();
     }
diff --git a/src/Acorn/Net/PacketHandlers/Chest/ChestAddClientPacketHandler.cs b/src/Acorn/Net/PacketHandlers/Chest/ChestAddClientPacketHandler.cs
@@ -124,7 +124,7 @@ await player.Send(new ChestReplyServerPacket
 
         // Notify other players near the chest
         var agreePacket = new ChestAgreeServerPacket { Items = chestItems };
-        foreach (var otherPlayer in player.CurrentMap.Players.Where(p => p != player))
+        foreach (var otherPlayer in player.CurrentMap.Players.Values.Where(p => p != player))
         {
             if (otherPlayer.Character == null) continue;
 
diff --git a/src/Acorn/Net/PacketHandlers/Chest/ChestTakeClientPacketHandler.cs b/src/Acorn/Net/PacketHandlers/Chest/ChestTakeClientPacketHandler.cs
@@ -126,7 +126,7 @@ await player.Send(new ChestGetServerPacket
 
         // Notify other players near the chest
         var agreePacket = new ChestAgreeServerPacket { Items = chestItems };
-        foreach (var otherPlayer in player.CurrentMap.Players.Where(p => p != player))
+        foreach (var otherPlayer in player.CurrentMap.Players.Values.Where(p => p != player))
         {
             if (otherPlayer.Character == null) continue;
 
diff --git a/src/Acorn/Net/PacketHandlers/Player/AttackUseClientPacketHandler.cs b/src/Acorn/Net/PacketHandlers/Player/AttackUseClientPacketHandler.cs
@@ -56,7 +56,7 @@ public async Task HandleAsync(PlayerState playerState, AttackUseClientPacket pac
         if (playerState.Character is not null)
         {
             var nextCoords = playerState.Character.NextCoords();
-            var target = playerState.CurrentMap.Npcs.FirstOrDefault(x =>
+            var target = playerState.CurrentMap.Npcs.Values.FirstOrDefault(x =>
                 !x.IsDead &&
                 x.X == nextCoords.X && x.Y == nextCoords.Y &&
                 x.Data.Type is NpcType.Aggressive or NpcType.Passive);
@@ -76,7 +76,7 @@ public async Task HandleAsync(PlayerState playerState, AttackUseClientPacket pac
                 target.AddOpponent(playerState.SessionId, damage);
             }
 
-            var npcIndex = playerState.CurrentMap.Npcs.ToList().IndexOf(target);
+            var npcIndex = playerState.CurrentMap.Npcs.Values.ToList().IndexOf(target);
             var hpPercentage = (int)Math.Max((double)target.Hp / target.Data.Hp * 100, 0);
 
             await playerState.CurrentMap.BroadcastPacket(new NpcReplyServerPacket
diff --git a/src/Acorn/Net/PacketHandlers/Player/FacePlayerClientPacketHandler.cs b/src/Acorn/Net/PacketHandlers/Player/FacePlayerClientPacketHandler.cs
@@ -21,7 +21,7 @@ public async Task HandleAsync(PlayerState playerState,
             return;
         }
 
-        var broadcast = playerState.CurrentMap.Players.Select(player =>
+        var broadcast = playerState.CurrentMap.Players.Values.Select(player =>
             player.Send(new FacePlayerServerPacket
             {
                 Direction = packet.Direction,
diff --git a/src/Acorn/Net/PacketHandlers/Player/PlayerRangeRequestClientPacketHandler.cs b/src/Acorn/Net/PacketHandlers/Player/PlayerRangeRequestClientPacketHandler.cs
@@ -19,7 +19,7 @@ await playerState.Send(new PlayersListServerPacket
         {
             PlayersList = new PlayersList
             {
-                Players = playerState.CurrentMap.Players.Select(x => x.Character?.AsOnlinePlayer()).ToList()
+                Players = playerState.CurrentMap.Players.Values.Select(x => x.Character?.AsOnlinePlayer()).ToList()
             }
         });
     }
diff --git a/src/Acorn/Net/PacketHandlers/Player/Talk/SpawnNpcCommandHandler.cs b/src/Acorn/Net/PacketHandlers/Player/Talk/SpawnNpcCommandHandler.cs
@@ -110,7 +110,7 @@ await _notifications.ServerAnnouncement(playerState,
                 IsAdminSpawned = true
             };
 
-            playerState.CurrentMap.Npcs.Add(npc);
+            playerState.CurrentMap.Npcs.TryAdd(npc.Id, npc);
         }
 
         await _notifications.ServerAnnouncement(playerState,
diff --git a/src/Acorn/Net/PacketHandlers/Player/Talk/WiseManTalkHandler.cs b/src/Acorn/Net/PacketHandlers/Player/Talk/WiseManTalkHandler.cs
@@ -77,7 +77,7 @@ public bool TryHandleMessage(PlayerState playerState, string message)
             // Find Wise Man NPC on the map
             var map = playerState.CurrentMap;
             var wiseManNpc =
-                map.Npcs.FirstOrDefault(n => n.Data.Name.Contains("Wise Man", StringComparison.OrdinalIgnoreCase));
+                map.Npcs.Values.FirstOrDefault(n => n.Data.Name.Contains("Wise Man", StringComparison.OrdinalIgnoreCase));
             if (wiseManNpc == null)
             {
                 _logger.LogWarning("No Wise Man NPC found on map for player {Player}", playerState.Character.Name);
diff --git a/src/Acorn/Net/PacketHandlers/Trade/TradeAcceptClientPacketHandler.cs b/src/Acorn/Net/PacketHandlers/Trade/TradeAcceptClientPacketHandler.cs
@@ -33,7 +33,7 @@ public async Task HandleAsync(PlayerState player, TradeAcceptClientPacket packet
         var partnerPlayerId = packet.PlayerId;
 
         // Find partner player who should have requested to trade with us
-        var partnerPlayer = player.CurrentMap.Players.FirstOrDefault(p => p.SessionId == partnerPlayerId);
+        var partnerPlayer = player.CurrentMap.Players.Values.FirstOrDefault(p => p.SessionId == partnerPlayerId);
         if (partnerPlayer?.Character == null)
         {
             logger.LogDebug("Partner player {PartnerId} not found on map", partnerPlayerId);
diff --git a/src/Acorn/Net/PacketHandlers/Trade/TradeRequestClientPacketHandler.cs b/src/Acorn/Net/PacketHandlers/Trade/TradeRequestClientPacketHandler.cs
@@ -29,7 +29,7 @@ public async Task HandleAsync(PlayerState player, TradeRequestClientPacket packe
         var targetPlayerId = packet.PlayerId;
 
         // Find target player on same map
-        var targetPlayer = player.CurrentMap.Players.FirstOrDefault(p => p.SessionId == targetPlayerId);
+        var targetPlayer = player.CurrentMap.Players.Values.FirstOrDefault(p => p.SessionId == targetPlayerId);
         if (targetPlayer?.Character == null)
         {
             logger.LogDebug("Target player {TargetId} not found on map", targetPlayerId);
diff --git a/src/Acorn/World/Map/MapState.cs b/src/Acorn/World/Map/MapState.cs
@@ -108,26 +108,26 @@ public MapState(
                 BehaviorType = npc.SpawnType == 7 ? NpcBehaviorType.Stationary : NpcBehaviorType.Wander
             };
 
-            Npcs.Add(npcState);
+            Npcs.TryAdd(npcState.Id, npcState);
         }
     }
 
     public int Id { get; set; }
     public Emf Data { get; set; }
 
-    public ConcurrentBag<NpcState> Npcs { get; set; } = new();
-    public ConcurrentBag<PlayerState> Players { get; set; } = new();
+    public ConcurrentDictionary<int, NpcState> Npcs { get; set; } = new();
+    public ConcurrentDictionary<int, PlayerState> Players { get; set; } = new();
     public ConcurrentDictionary<int, MapItem> Items { get; set; } = new();
     public ConcurrentDictionary<Coords, MapChest> Chests { get; set; } = new();
 
     public bool HasPlayer(PlayerState player)
     {
-        return Players.Contains(player);
+        return Players.ContainsKey(player.SessionId);
     }
 
     public IEnumerable<PlayerState> PlayersExcept(PlayerState? except)
     {
-        return Players.Where(x => except is null || x != except);
+        return Players.Values.Where(x => except is null || x.SessionId != except.SessionId);
     }
 
     public async Task BroadcastPacket(IPacket packet, PlayerState? except = null)
@@ -157,7 +157,8 @@ public NearbyInfo AsNearbyInfo(PlayerState? except = null, WarpEffect warpEffect
 
     public List<NpcMapInfo> AsNpcMapInfo()
     {
-        return Npcs.Select((x, i) => (npc: x, index: i))
+        return Npcs.Values
+            .Select((npc, index) => (npc, index))
             .Where(t => !t.npc.IsDead)
             .Select(t => t.npc.AsNpcMapInfo(t.index))
             .Take(252) // EO Protocol limit: NpcMapInfo uses byte field, max 252 NPCs
@@ -173,10 +174,7 @@ public async Task NotifyEnter(PlayerState player, WarpEffect warpEffect = WarpEf
 
         player.Character.Map = Id;
 
-        if (!Players.Contains(player))
-        {
-            Players.Add(player);
-        }
+        Players.TryAdd(player.SessionId, player);
 
         await BroadcastPacket(new PlayersAgreeServerPacket
         {
@@ -188,16 +186,16 @@ await BroadcastPacket(new PlayersAgreeServerPacket
 
     public async Task NotifyLeave(PlayerState player, WarpEffect warpEffect = WarpEffect.None)
     {
-        Players = new ConcurrentBag<PlayerState>(Players.Where(p => p != player));
+        Players.TryRemove(player.SessionId, out _);
 
-        await _broadcastService.NotifyPlayerLeave(Players, player, warpEffect);
+        await _broadcastService.NotifyPlayerLeave(Players.Values.ToList(), player, warpEffect);
     }
 
     public bool IsTileOccupied(Coords coords)
     {
-        return Players.Any(p => p.Character != null && !p.Character.Hidden &&
+        return Players.Values.Any(p => p.Character != null && !p.Character.Hidden &&
                                 p.Character.X == coords.X && p.Character.Y == coords.Y)
-               || Npcs.Any(n => n.X == coords.X && n.Y == coords.Y);
+               || Npcs.Values.Any(n => n.X == coords.X && n.Y == coords.Y);
     }
 
     public int GetNextItemIndex(int seed = 1)
@@ -222,7 +220,7 @@ public Task<bool> StandFromChair(PlayerState player)
 
     public async Task Tick()
     {
-        if (Players.Any() is false)
+        if (Players.IsEmpty)
         {
             return;
         }
diff --git a/src/Acorn/World/Services/Map/MapController.cs b/src/Acorn/World/Services/Map/MapController.cs
@@ -195,7 +195,7 @@ public async Task ProcessNpcRespawnsAsync(MapState map)
                         npc.Data.Name, npc.Id, npc.X, npc.Y);
 
                     // Broadcast respawn to all players on map
-                    var npcIndex = map.Npcs.ToList().IndexOf(npc);
+                    var npcIndex = map.Npcs.Values.ToList().IndexOf(npc);
                     await map.BroadcastPacket(new NpcAgreeServerPacket
                     {
                         Npcs = new List<NpcMapInfo>
@@ -210,8 +210,8 @@ await map.BroadcastPacket(new NpcAgreeServerPacket
 
     public async Task<HashSet<int>> ProcessNpcActionsAsync(MapState map)
     {
-        var aliveNpcs = map.Npcs.Where(n => !n.IsDead).ToList();
-        var npcList = map.Npcs.ToList();
+        var aliveNpcs = map.Npcs.Values.Where(n => !n.IsDead).ToList();
+        var npcList = map.Npcs.Values.ToList();
 
         var positionUpdates = new List<NpcUpdatePosition>();
         var attackUpdates = new List<NpcUpdateAttack>();
@@ -257,7 +257,7 @@ public async Task<HashSet<int>> ProcessNpcActionsAsync(MapState map)
         // This prevents the client from receiving updates for NPCs it has removed from memory
         if (positionUpdates.Count > 0 || attackUpdates.Count > 0)
         {
-            var playersWithCharacters = map.Players.Where(p => p.Character != null).ToList();
+            var playersWithCharacters = map.Players.Values.Where(p => p.Character != null).ToList();
 
             // Filter position updates to only include NPCs within client range of at least one player
             var filteredPositionUpdates = positionUpdates
@@ -286,7 +286,7 @@ await map.BroadcastPacket(new NpcPlayerServerPacket
 
             if (attack.Killed == PlayerKilledState.Killed)
             {
-                var deadPlayer = map.Players.FirstOrDefault(p => p.SessionId == attack.PlayerId);
+                var deadPlayer = map.Players.Values.FirstOrDefault(p => p.SessionId == attack.PlayerId);
                 if (deadPlayer?.Character != null)
                 {
                     tasks.Add(_playerController.DieAsync(deadPlayer));
diff --git a/src/Acorn/World/Services/WiseManQueueService.cs b/src/Acorn/World/Services/WiseManQueueService.cs
@@ -54,7 +54,7 @@ public bool TryEnqueue(WiseManRequest request)
 
         // Find Wise Man NPC on the map
         var wiseManNpc =
-            map.Npcs.FirstOrDefault(n => n.Data.Name.Contains("Wise Man", StringComparison.OrdinalIgnoreCase));
+            map.Npcs.Values.FirstOrDefault(n => n.Data.Name.Contains("Wise Man", StringComparison.OrdinalIgnoreCase));
         if (wiseManNpc == null)
         {
             return false;
@@ -191,7 +191,7 @@ private async Task SendWiseManMessageAsync(PlayerState player, string message, N
                 }
             }
 
-            var npcIndex = player.CurrentMap.Npcs.ToList().IndexOf(wiseManNpc);
+            var npcIndex = player.CurrentMap.Npcs.Values.ToList().IndexOf(wiseManNpc);
             var chatUpdate = new NpcUpdateChat
             {
                 NpcIndex = npcIndex,
@@ -201,7 +201,7 @@ private async Task SendWiseManMessageAsync(PlayerState player, string message, N
             {
                 Chats = [chatUpdate]
             };
-            foreach (var mapPlayer in player.CurrentMap.Players)
+            foreach (var mapPlayer in player.CurrentMap.Players.Values)
             {
                 try
                 {
diff --git a/src/Acorn/World/WorldHostedService.cs b/src/Acorn/World/WorldHostedService.cs
@@ -1,6 +1,7 @@
 ﻿using System.Timers;
 using Acorn.Options;
 using Microsoft.Extensions.Hosting;
+using Microsoft.Extensions.Logging;
 using Microsoft.Extensions.Options;
 using Timer = System.Timers.Timer;
 
@@ -10,12 +11,14 @@ internal class WorldHostedService : IHostedService
 {
     private readonly Timer _timer;
     private readonly WorldState _world;
+    private readonly ILogger<WorldHostedService> _logger;
 
-    public WorldHostedService(IOptions<ServerOptions> options, WorldState world)
+    public WorldHostedService(IOptions<ServerOptions> options, WorldState world, ILogger<WorldHostedService> logger)
     {
         _world = world;
+        _logger = logger;
         _timer = new Timer(options.Value.TickRate);
-        _timer.Elapsed += Tick!;
+        _timer.Elapsed += OnTick;
     }
 
     public Task StartAsync(CancellationToken cancellationToken)
@@ -30,12 +33,19 @@ public Task StopAsync(CancellationToken cancellationToken)
         return Task.CompletedTask;
     }
 
-    private void Tick(object sender, ElapsedEventArgs args)
+    private async void OnTick(object sender, ElapsedEventArgs args)
     {
-        var tickTasks = _world
-            .Maps
-            .Select(x => x.Value.Tick());
+        try
+        {
+            var tickTasks = _world
+                .Maps
+                .Select(x => x.Value.Tick());
 
-        Task.WhenAll(tickTasks).GetAwaiter().GetResult();
+            await Task.WhenAll(tickTasks);
+        }
+        catch (Exception ex)
+        {
+            _logger.LogError(ex, "Error during world tick");
+        }
     }
 }
diff --git a/src/Acorn/World/WorldState.cs b/src/Acorn/World/WorldState.cs
diff --git a/src/Acorn/World/WorldStateQueries.cs b/src/Acorn/World/WorldStateQueries.cs

Original file line number	Diff line number	Diff line change
`@@ -124,7 +124,7 @@ await player.Send(new ChestReplyServerPacket`
`124`	`124`
`125`	`125`	`// Notify other players near the chest`
`126`	`126`	`var agreePacket = new ChestAgreeServerPacket { Items = chestItems };`
`127`		`- foreach (var otherPlayer in player.CurrentMap.Players.Where(p => p != player))`
	`127`	`+ foreach (var otherPlayer in player.CurrentMap.Players.Values.Where(p => p != player))`
`128`	`128`	`{`
`129`	`129`	`if (otherPlayer.Character == null) continue;`
`130`	`130`
Original file line number	Diff line number	Diff line change
`@@ -126,7 +126,7 @@ await player.Send(new ChestGetServerPacket`
`126`	`126`
`127`	`127`	`// Notify other players near the chest`
`128`	`128`	`var agreePacket = new ChestAgreeServerPacket { Items = chestItems };`
`129`		`- foreach (var otherPlayer in player.CurrentMap.Players.Where(p => p != player))`
	`129`	`+ foreach (var otherPlayer in player.CurrentMap.Players.Values.Where(p => p != player))`
`130`	`130`	`{`
`131`	`131`	`if (otherPlayer.Character == null) continue;`
`132`	`132`
Original file line number	Diff line number	Diff line change
`@@ -21,7 +21,7 @@ public async Task HandleAsync(PlayerState playerState,`
`21`	`21`	`return;`
`22`	`22`	`}`
`23`	`23`
`24`		`- var broadcast = playerState.CurrentMap.Players.Select(player =>`
	`24`	`+ var broadcast = playerState.CurrentMap.Players.Values.Select(player =>`
`25`	`25`	`player.Send(new FacePlayerServerPacket`
`26`	`26`	`{`
`27`	`27`	`Direction = packet.Direction,`
Original file line number	Diff line number	Diff line change
`@@ -19,7 +19,7 @@ await playerState.Send(new PlayersListServerPacket`
`19`	`19`	`{`
`20`	`20`	`PlayersList = new PlayersList`
`21`	`21`	`{`
`22`		`- Players = playerState.CurrentMap.Players.Select(x => x.Character?.AsOnlinePlayer()).ToList()`
	`22`	`+ Players = playerState.CurrentMap.Players.Values.Select(x => x.Character?.AsOnlinePlayer()).ToList()`
`23`	`23`	`}`
`24`	`24`	`});`
`25`	`25`	`}`
Original file line number	Diff line number	Diff line change
`@@ -110,7 +110,7 @@ await _notifications.ServerAnnouncement(playerState,`
`110`	`110`	`IsAdminSpawned = true`
`111`	`111`	`};`
`112`	`112`
`113`		`- playerState.CurrentMap.Npcs.Add(npc);`
	`113`	`+ playerState.CurrentMap.Npcs.TryAdd(npc.Id, npc);`
`114`	`114`	`}`
`115`	`115`
`116`	`116`	`await _notifications.ServerAnnouncement(playerState,`
Original file line number	Diff line number	Diff line change
`@@ -77,7 +77,7 @@ public bool TryHandleMessage(PlayerState playerState, string message)`
`77`	`77`	`// Find Wise Man NPC on the map`
`78`	`78`	`var map = playerState.CurrentMap;`
`79`	`79`	`var wiseManNpc =`
`80`		`- map.Npcs.FirstOrDefault(n => n.Data.Name.Contains("Wise Man", StringComparison.OrdinalIgnoreCase));`
	`80`	`+ map.Npcs.Values.FirstOrDefault(n => n.Data.Name.Contains("Wise Man", StringComparison.OrdinalIgnoreCase));`
`81`	`81`	`if (wiseManNpc == null)`
`82`	`82`	`{`
`83`	`83`	`_logger.LogWarning("No Wise Man NPC found on map for player {Player}", playerState.Character.Name);`
Original file line number	Diff line number	Diff line change
`@@ -33,7 +33,7 @@ public async Task HandleAsync(PlayerState player, TradeAcceptClientPacket packet`
`33`	`33`	`var partnerPlayerId = packet.PlayerId;`
`34`	`34`
`35`	`35`	`// Find partner player who should have requested to trade with us`
`36`		`- var partnerPlayer = player.CurrentMap.Players.FirstOrDefault(p => p.SessionId == partnerPlayerId);`
	`36`	`+ var partnerPlayer = player.CurrentMap.Players.Values.FirstOrDefault(p => p.SessionId == partnerPlayerId);`
`37`	`37`	`if (partnerPlayer?.Character == null)`
`38`	`38`	`{`
`39`	`39`	`logger.LogDebug("Partner player {PartnerId} not found on map", partnerPlayerId);`
Original file line number	Diff line number	Diff line change
`@@ -29,7 +29,7 @@ public async Task HandleAsync(PlayerState player, TradeRequestClientPacket packe`
`29`	`29`	`var targetPlayerId = packet.PlayerId;`
`30`	`30`
`31`	`31`	`// Find target player on same map`
`32`		`- var targetPlayer = player.CurrentMap.Players.FirstOrDefault(p => p.SessionId == targetPlayerId);`
	`32`	`+ var targetPlayer = player.CurrentMap.Players.Values.FirstOrDefault(p => p.SessionId == targetPlayerId);`
`33`	`33`	`if (targetPlayer?.Character == null)`
`34`	`34`	`{`
`35`	`35`	`logger.LogDebug("Target player {TargetId} not found on map", targetPlayerId);`