WIP Make networking code thread-safe and refactor update ID

Replaces direct increments of LastClientListUpdateID with a thread-safe IncrementLastClientListUpdateID method and uses Interlocked for atomic operations. Refactors EntitySpawner to lock access to the spawn/remove queue for thread safety. Updates INetSerializableStruct to use concurrent collections for cached variables and type behaviors, improving thread safety in networking code.

Barotrauma/BarotraumaServer/ServerSource/Networking/Client.cs

@@ -126,7 +126,7 @@ namespace Barotrauma.Networking
                 if (!MathUtils.NearlyEqual(karma, syncedKarma, 10.0f))
                 {
                     syncedKarma = karma;
-                    GameMain.NetworkMember.LastClientListUpdateID++;
+                    GameMain.NetworkMember.IncrementLastClientListUpdateID();
                 }
             }
         }

Barotrauma/BarotraumaServer/ServerSource/Networking/FileTransfer/FileSender.cs

@@ -174,7 +174,7 @@ namespace Barotrauma.Networking
             StartTime = DateTime.Now;
 
             OnStarted(transfer);
-            GameMain.Server.LastClientListUpdateID++;
+            GameMain.Server.IncrementLastClientListUpdateID();
 
             return transfer;
         }
@@ -204,7 +204,7 @@ namespace Barotrauma.Networking
 
             if (numRemoved > 0 || endedTransfers.Count > 0)
             {
-                GameMain.Server.LastClientListUpdateID++;
+                GameMain.Server.IncrementLastClientListUpdateID();
             }
         }
 

Barotrauma/BarotraumaServer/ServerSource/Networking/GameServer.cs

@@ -327,7 +327,7 @@ namespace Barotrauma.Networking
                 }
             }
 
-            LastClientListUpdateID++;
+            IncrementLastClientListUpdateID();
 
             if (newClient.Connection == OwnerConnection && OwnerConnection != null)
             {
@@ -3222,7 +3222,7 @@ namespace Barotrauma.Networking
             initiatedStartGame = false;
             GameMain.ResetFrameTime();
 
-            LastClientListUpdateID++;
+            IncrementLastClientListUpdateID();
 
             roundStartTime = DateTime.Now;
 
@@ -3532,7 +3532,7 @@ namespace Barotrauma.Networking
                 {
                     var coolDownRemaining = Client.NameChangeCoolDown - timeSinceNameChange;
                     SendDirectChatMessage($"ServerMessage.NameChangeFailedCooldownActive~[seconds]={(int)coolDownRemaining.TotalSeconds}", c);
-                    LastClientListUpdateID++;
+                    IncrementLastClientListUpdateID();
                     //increment the ID to make sure the current server-side name is treated as the "latest",
                     //and the client correctly reverts back to the old name
                     c.NameId++;
@@ -3545,7 +3545,7 @@ namespace Barotrauma.Networking
 
             if (result != null)
             {
-                LastClientListUpdateID++;
+                IncrementLastClientListUpdateID();
                 return result.Value;
             }
 
@@ -3562,14 +3562,14 @@ namespace Barotrauma.Networking
                 c.Name = newName;
                 c.RejectedName = string.Empty;
                 SendChatMessage($"ServerMessage.NameChangeSuccessful~[oldname]={oldName}~[newname]={newName}", ChatMessageType.Server);
-                LastClientListUpdateID++;
+                IncrementLastClientListUpdateID();
                 return true;
             }
             else
             {
                 //update client list even if the name cannot be changed to the one sent by the client,
                 //so the client will be informed what their actual name is
-                LastClientListUpdateID++;
+                IncrementLastClientListUpdateID();
                 return false;
             }
         }
@@ -4857,7 +4857,7 @@ namespace Barotrauma.Networking
         private void UpdateClientLobbies()
         {
             // Triggers a call to WriteClientList(), which causes clients to call GameClient.ReadClientList()
-            LastClientListUpdateID++;
+            IncrementLastClientListUpdateID();
         }
 
         private List<Client> GetPlayingClients()

Barotrauma/BarotraumaServer/ServerSource/Networking/KarmaManager.cs

@@ -163,7 +163,7 @@ namespace Barotrauma
                 {
                     client.Character.CharacterHealth.ApplyAffliction(null, new Affliction(herpesAffliction, herpesStrength));
                     GameServer.Log($"{GameServer.ClientLogName(client)} has contracted space herpes due to low karma.", ServerLog.MessageType.Karma);
-                    GameMain.NetworkMember.LastClientListUpdateID++;
+                    GameMain.NetworkMember.IncrementLastClientListUpdateID();
                 }
                 else if (existingAffliction != null)
                 {

Barotrauma/BarotraumaShared/SharedSource/Networking/Client.cs

@@ -62,7 +62,7 @@ namespace Barotrauma.Networking
                     DebugConsole.Log($"Changed client {Name}'s team to {teamID}.");
                     if (GameMain.NetworkMember != null && GameMain.NetworkMember.IsServer)
                     {
-                        GameMain.NetworkMember.LastClientListUpdateID++;
+                        GameMain.NetworkMember.IncrementLastClientListUpdateID();
                     }
                     teamID = value;
                 }
@@ -86,7 +86,7 @@ namespace Barotrauma.Networking
             {
                 if (GameMain.NetworkMember != null && GameMain.NetworkMember.IsServer)
                 {
-                    GameMain.NetworkMember.LastClientListUpdateID++;
+                    GameMain.NetworkMember.IncrementLastClientListUpdateID();
                     if (value != null)
                     {
                         CharacterID = value.ID;
@@ -154,7 +154,7 @@ namespace Barotrauma.Networking
 #endif
                 if (GameMain.NetworkMember != null && GameMain.NetworkMember.IsServer)
                 {
-                    GameMain.NetworkMember.LastClientListUpdateID++;
+                    GameMain.NetworkMember.IncrementLastClientListUpdateID();
                 }
             }
         }
@@ -178,7 +178,7 @@ namespace Barotrauma.Networking
             {
                 if (GameMain.NetworkMember != null && GameMain.NetworkMember.IsServer)
                 {
-                    GameMain.NetworkMember.LastClientListUpdateID++;
+                    GameMain.NetworkMember.IncrementLastClientListUpdateID();
                 }
                 inGame = value;
             }

Barotrauma/BarotraumaShared/SharedSource/Networking/EntitySpawner.cs

@@ -5,6 +5,7 @@ using Microsoft.Xna.Framework;
 using System;
 using System.Collections.Generic;
 using System.Linq;
+using System.Threading;
 
 namespace Barotrauma
 {
@@ -205,6 +206,7 @@ namespace Barotrauma
         }
 
         private readonly Queue<Either<IEntitySpawnInfo, Entity>> spawnOrRemoveQueue;
+        private readonly object spawnOrRemoveQueueLock = new object();
 
         public abstract class SpawnOrRemove : NetEntityEvent.IData
         {
@@ -282,7 +284,10 @@ namespace Barotrauma
                 GameAnalyticsManager.AddErrorEventOnce("EntitySpawner.AddToSpawnQueue1:ItemPrefabNull", GameAnalyticsManager.ErrorSeverity.Error, errorMsg);
                 return;
             }
-            spawnOrRemoveQueue.Enqueue(new ItemSpawnInfo(itemPrefab, worldPosition, onSpawned, condition, quality));
+            lock (spawnOrRemoveQueueLock)
+            {
+                spawnOrRemoveQueue.Enqueue(new ItemSpawnInfo(itemPrefab, worldPosition, onSpawned, condition, quality));
+            }
         }
 
         public void AddItemToSpawnQueue(ItemPrefab itemPrefab, Vector2 position, Submarine sub, float? condition = null, int? quality = null, Action<Item> onSpawned = null)
@@ -295,7 +300,10 @@ namespace Barotrauma
                 GameAnalyticsManager.AddErrorEventOnce("EntitySpawner.AddToSpawnQueue2:ItemPrefabNull", GameAnalyticsManager.ErrorSeverity.Error, errorMsg);
                 return;
             }
-            spawnOrRemoveQueue.Enqueue(new ItemSpawnInfo(itemPrefab, position, sub, onSpawned, condition, quality));
+            lock (spawnOrRemoveQueueLock)
+            {
+                spawnOrRemoveQueue.Enqueue(new ItemSpawnInfo(itemPrefab, position, sub, onSpawned, condition, quality));
+            }
         }
 
         public void AddItemToSpawnQueue(ItemPrefab itemPrefab, Inventory inventory, float? condition = null, int? quality = null, Action<Item> onSpawned = null, bool spawnIfInventoryFull = true, bool ignoreLimbSlots = false, InvSlotType slot = InvSlotType.None)
@@ -308,12 +316,15 @@ namespace Barotrauma
                 GameAnalyticsManager.AddErrorEventOnce("EntitySpawner.AddToSpawnQueue3:ItemPrefabNull", GameAnalyticsManager.ErrorSeverity.Error, errorMsg);
                 return;
             }
-            spawnOrRemoveQueue.Enqueue(new ItemSpawnInfo(itemPrefab, inventory, onSpawned, condition, quality) 
+            lock (spawnOrRemoveQueueLock)
-            { 
+            {
-                SpawnIfInventoryFull = spawnIfInventoryFull, 
+                spawnOrRemoveQueue.Enqueue(new ItemSpawnInfo(itemPrefab, inventory, onSpawned, condition, quality) 
-                IgnoreLimbSlots = ignoreLimbSlots,
+                { 
-                Slot = slot
+                    SpawnIfInventoryFull = spawnIfInventoryFull, 
-            });
+                    IgnoreLimbSlots = ignoreLimbSlots,
+                    Slot = slot
+                });
+            }
         }
 
         public void AddCharacterToSpawnQueue(Identifier speciesName, Vector2 worldPosition, Action<Character> onSpawn = null)
@@ -326,7 +337,10 @@ namespace Barotrauma
                 GameAnalyticsManager.AddErrorEventOnce("EntitySpawner.AddToSpawnQueue4:SpeciesNameNullOrEmpty", GameAnalyticsManager.ErrorSeverity.Error, errorMsg);
                 return;
             }
-            spawnOrRemoveQueue.Enqueue(new CharacterSpawnInfo(speciesName, worldPosition, onSpawn));
+            lock (spawnOrRemoveQueueLock)
+            {
+                spawnOrRemoveQueue.Enqueue(new CharacterSpawnInfo(speciesName, worldPosition, onSpawn));
+            }
         }
 
         public void AddCharacterToSpawnQueue(Identifier speciesName, Vector2 position, Submarine sub, Action<Character> onSpawn = null)
@@ -339,7 +353,10 @@ namespace Barotrauma
                 GameAnalyticsManager.AddErrorEventOnce("EntitySpawner.AddToSpawnQueue5:SpeciesNameNullOrEmpty", GameAnalyticsManager.ErrorSeverity.Error, errorMsg);
                 return;
             }
-            spawnOrRemoveQueue.Enqueue(new CharacterSpawnInfo(speciesName, position, sub, onSpawn));
+            lock (spawnOrRemoveQueueLock)
+            {
+                spawnOrRemoveQueue.Enqueue(new CharacterSpawnInfo(speciesName, position, sub, onSpawn));
+            }
         }
 
         public void AddCharacterToSpawnQueue(Identifier speciesName, Vector2 worldPosition, CharacterInfo characterInfo, Action<Character> onSpawn = null)
@@ -352,7 +369,10 @@ namespace Barotrauma
                 GameAnalyticsManager.AddErrorEventOnce("EntitySpawner.AddToSpawnQueue4:SpeciesNameNullOrEmpty", GameAnalyticsManager.ErrorSeverity.Error, errorMsg);
                 return;
             }
-            spawnOrRemoveQueue.Enqueue(new CharacterSpawnInfo(speciesName, worldPosition, characterInfo, onSpawn));
+            lock (spawnOrRemoveQueueLock)
+            {
+                spawnOrRemoveQueue.Enqueue(new CharacterSpawnInfo(speciesName, worldPosition, characterInfo, onSpawn));
+            }
         }
 
         public void AddEntityToRemoveQueue(Entity entity)
@@ -375,7 +395,10 @@ namespace Barotrauma
 #endif
             }
 
-            spawnOrRemoveQueue.Enqueue(entity);
+            lock (spawnOrRemoveQueueLock)
+            {
+                spawnOrRemoveQueue.Enqueue(entity);
+            }
         }
 
         public void AddItemToRemoveQueue(Item item)
@@ -383,7 +406,10 @@ namespace Barotrauma
             if (GameMain.NetworkMember != null && GameMain.NetworkMember.IsClient) { return; }
             if (IsInRemoveQueue(item) || item.Removed) { return; }
 
-            spawnOrRemoveQueue.Enqueue(item);
+            lock (spawnOrRemoveQueueLock)
+            {
+                spawnOrRemoveQueue.Enqueue(item);
+            }
             item.IsInRemoveQueue = true;
 
             foreach (var containedItem in item.ContainedItems)
@@ -400,11 +426,14 @@ namespace Barotrauma
         /// </summary>
         public bool IsInSpawnQueue(Predicate<IEntitySpawnInfo> predicate)
         {
-            foreach (var spawnOrRemove in spawnOrRemoveQueue)
+            lock (spawnOrRemoveQueueLock)
             {
-                if (spawnOrRemove.TryGet(out IEntitySpawnInfo spawnInfo) && predicate(spawnInfo)) { return true; }
+                foreach (var spawnOrRemove in spawnOrRemoveQueue)
+                {
+                    if (spawnOrRemove.TryGet(out IEntitySpawnInfo spawnInfo) && predicate(spawnInfo)) { return true; }
+                }
+                return false;
             }
-            return false;
         }
 
         /// <summary>
@@ -412,29 +441,40 @@ namespace Barotrauma
         /// </summary>
         public int CountSpawnQueue(Predicate<IEntitySpawnInfo> predicate)
         {
-            int count = 0;
+            lock (spawnOrRemoveQueueLock)
-            foreach (var spawnOrRemove in spawnOrRemoveQueue)
             {
-                if (spawnOrRemove.TryGet(out IEntitySpawnInfo spawnInfo) && predicate(spawnInfo)) { count++; }
+                int count = 0;
+                foreach (var spawnOrRemove in spawnOrRemoveQueue)
+                {
+                    if (spawnOrRemove.TryGet(out IEntitySpawnInfo spawnInfo) && predicate(spawnInfo)) { count++; }
+                }
+                return count;
             }
-            return count;
         }
 
         public bool IsInRemoveQueue(Entity entity)
         {
-            foreach (var spawnOrRemove in spawnOrRemoveQueue)
+            lock (spawnOrRemoveQueueLock)
             {
-                if (spawnOrRemove.TryGet(out Entity entityToRemove) && entityToRemove == entity) { return true; }
+                foreach (var spawnOrRemove in spawnOrRemoveQueue)
+                {
+                    if (spawnOrRemove.TryGet(out Entity entityToRemove) && entityToRemove == entity) { return true; }
+                }
+                return false;
             }
-            return false;
         }
 
         public void Update(bool createNetworkEvents = true)
         {
             if (GameMain.NetworkMember is { IsClient: true }) { return; }
-            while (spawnOrRemoveQueue.Count > 0)
+            while (true)
             {
-                var spawnOrRemove = spawnOrRemoveQueue.Dequeue();
+                Either<IEntitySpawnInfo, Entity> spawnOrRemove;
+                lock (spawnOrRemoveQueueLock)
+                {
+                    if (spawnOrRemoveQueue.Count == 0) { break; }
+                    spawnOrRemove = spawnOrRemoveQueue.Dequeue();
+                }
                 if (spawnOrRemove.TryGet(out Entity entityToRemove))
                 {
                     if (entityToRemove is Item item)
@@ -465,7 +505,10 @@ namespace Barotrauma
 
         public void Reset()
         {
-            spawnOrRemoveQueue.Clear();
+            lock (spawnOrRemoveQueueLock)
+            {
+                spawnOrRemoveQueue.Clear();
+            }
 #if CLIENT
             receivedEvents.Clear();
 #endif

Barotrauma/BarotraumaShared/SharedSource/Networking/INetSerializableStruct.cs

@@ -1,5 +1,6 @@
 #nullable enable
 using System;
+using System.Collections.Concurrent;
 using System.Collections.Generic;
 using System.Collections.Immutable;
 using System.Diagnostics.CodeAnalysis;
@@ -146,10 +147,10 @@ namespace Barotrauma
             }
         }
 
-        private static readonly Dictionary<Type, ImmutableArray<CachedReflectedVariable>> CachedVariables = new Dictionary<Type, ImmutableArray<CachedReflectedVariable>>();
+        private static readonly ConcurrentDictionary<Type, ImmutableArray<CachedReflectedVariable>> CachedVariables = new ConcurrentDictionary<Type, ImmutableArray<CachedReflectedVariable>>();
 
-        private static readonly Dictionary<Type, IReadWriteBehavior> TypeBehaviors
+        private static readonly ConcurrentDictionary<Type, IReadWriteBehavior> TypeBehaviors
-            = new Dictionary<Type, IReadWriteBehavior>
+            = new ConcurrentDictionary<Type, IReadWriteBehavior>(new Dictionary<Type, IReadWriteBehavior>
             {
                 { typeof(Boolean), new ReadWriteBehavior<Boolean>(ReadBoolean, WriteBoolean) },
                 { typeof(Byte), new ReadWriteBehavior<Byte>(ReadByte, WriteByte) },
@@ -168,7 +169,7 @@ namespace Barotrauma
                 { typeof(Vector2), new ReadWriteBehavior<Vector2>(ReadVector2, WriteVector2) },
                 { typeof(SerializableDateTime), new ReadWriteBehavior<SerializableDateTime>(ReadSerializableDateTime, WriteSerializableDateTime) },
                 { typeof(NetLimitedString), new ReadWriteBehavior<NetLimitedString>(ReadNetLString, WriteNetLString) }
-            };
+            });
 
         private static readonly ImmutableDictionary<Predicate<Type>, Func<Type, IReadWriteBehavior>> BehaviorFactories = new Dictionary<Predicate<Type>, Func<Type, IReadWriteBehavior>>
         {
@@ -584,7 +585,11 @@ namespace Barotrauma
                 if (!predicate(type)) { continue; }
 
                 behavior = factory(type);
-                TypeBehaviors.Add(type, behavior);
+                // Use TryAdd for thread-safety; if another thread already added, use that value
+                if (!TypeBehaviors.TryAdd(type, behavior))
+                {
+                    behavior = TypeBehaviors[type];
+                }
                 return true;
             }
 
@@ -594,8 +599,11 @@ namespace Barotrauma
 
         public static ImmutableArray<CachedReflectedVariable> GetPropertiesAndFields(Type type)
         {
-            if (CachedVariables.TryGetValue(type, out var cached)) { return cached; }
+            return CachedVariables.GetOrAdd(type, static t => CreateCachedVariables(t));
+        }
 
+        private static ImmutableArray<CachedReflectedVariable> CreateCachedVariables(Type type)
+        {
             List<CachedReflectedVariable> variables = new List<CachedReflectedVariable>();
 
             IEnumerable<PropertyInfo> propertyInfos = type.GetProperties().Where(HasAttribute).Where(NotStatic);
@@ -633,7 +641,6 @@ namespace Barotrauma
             }
 
             ImmutableArray<CachedReflectedVariable> array = variables.All(v => v.HasOwnAttribute) ? variables.OrderBy(v => v.Attribute.OrderKey).ToImmutableArray() : variables.ToImmutableArray();
-            CachedVariables.Add(type, array);
             return array;
 
             bool HasAttribute(MemberInfo info) => (info.GetCustomAttribute<NetworkSerialize>() ?? type.GetCustomAttribute<NetworkSerialize>()) != null;
@@ -874,4 +881,4 @@ namespace Barotrauma
             }
         }
     }
-}
+}

Barotrauma/BarotraumaShared/SharedSource/Networking/NetworkMember.cs

@@ -1,6 +1,7 @@
 using Microsoft.Xna.Framework;
 using System;
 using System.Collections.Generic;
+using System.Threading;
 
 namespace Barotrauma.Networking
 {
@@ -186,10 +187,19 @@ namespace Barotrauma.Networking
     {
         protected const int MaxSubNameLengthInErrorMessages = 16;
 
+        private int lastClientListUpdateID;
         public UInt16 LastClientListUpdateID
         {
-            get;
+            get => (UInt16)Interlocked.CompareExchange(ref lastClientListUpdateID, 0, 0);
-            set;
+            set => Interlocked.Exchange(ref lastClientListUpdateID, value);
+        }
+
+        /// <summary>
+        /// Thread-safe increment of LastClientListUpdateID
+        /// </summary>
+        public void IncrementLastClientListUpdateID()
+        {
+            Interlocked.Increment(ref lastClientListUpdateID);
         }
 
         public abstract bool IsServer { get; }