Lock Server.LobbyInfo to prevent races with callback threads.

OpenRA.Game/Server/Server.cs

@@ -430,6 +430,8 @@ namespace OpenRA.Server
 				}
 
 				Action completeConnection = () =>
+				{
+					lock (LobbyInfo)
 					{
 						client.Slot = LobbyInfo.FirstEmptySlot();
 						client.IsAdmin = !LobbyInfo.Clients.Any(c1 => c1.IsAdmin);
@@ -494,6 +496,7 @@ namespace OpenRA.Server
 							SendOrderTo(newConn, "Message", TwoHumansRequiredText);
 						else if (Map.Players.Players.Where(p => p.Value.Playable).All(p => !p.Value.AllowBots))
 							SendOrderTo(newConn, "Message", "Bots have been disabled on this map.");
+					}
 				};
 
 				if (Type == ServerType.Local)
@@ -675,6 +678,8 @@ namespace OpenRA.Server
 		}
 
 		void InterpretServerOrder(Connection conn, Order o)
+		{
+			lock (LobbyInfo)
 			{
 				// Only accept handshake responses from unvalidated clients
 				// Anything else may be an attempt to exploit the server
@@ -858,6 +863,7 @@ namespace OpenRA.Server
 						}
 				}
 			}
+		}
 
 		public Session.Client GetClient(Connection conn)
 		{
@@ -865,6 +871,8 @@ namespace OpenRA.Server
 		}
 
 		public void DropClient(Connection toDrop)
+		{
+			lock (LobbyInfo)
 			{
 				if (!PreConns.Remove(toDrop))
 				{
@@ -915,6 +923,7 @@ namespace OpenRA.Server
 					if (Type != ServerType.Dedicated && dropClient.IsAdmin)
 						Shutdown();
 				}
+			}
 
 			try
 			{
@@ -924,6 +933,8 @@ namespace OpenRA.Server
 		}
 
 		public void SyncLobbyInfo()
+		{
+			lock (LobbyInfo)
 			{
 				if (State == ServerState.WaitingPlayers) // Don't do this while the game is running, it breaks things!
 					DispatchOrders(null, 0, Order.FromTargetString("SyncInfo", LobbyInfo.Serialize(), true).Serialize());
@@ -931,12 +942,15 @@ namespace OpenRA.Server
 				foreach (var t in serverTraits.WithInterface<INotifySyncLobbyInfo>())
 					t.LobbyInfoSynced(this);
 			}
+		}
 
 		public void SyncLobbyClients()
 		{
 			if (State != ServerState.WaitingPlayers)
 				return;
 
+			lock (LobbyInfo)
+			{
 				// TODO: Only need to sync the specific client that has changed to avoid conflicts!
 				var clientData = LobbyInfo.Clients.Select(client => client.Serialize()).ToList();
 
@@ -945,12 +959,15 @@ namespace OpenRA.Server
 				foreach (var t in serverTraits.WithInterface<INotifySyncLobbyInfo>())
 					t.LobbyInfoSynced(this);
 			}
+		}
 
 		public void SyncLobbySlots()
 		{
 			if (State != ServerState.WaitingPlayers)
 				return;
 
+			lock (LobbyInfo)
+			{
 				// TODO: Don't sync all the slots if just one changed!
 				var slotData = LobbyInfo.Slots.Select(slot => slot.Value.Serialize()).ToList();
 
@@ -959,12 +976,15 @@ namespace OpenRA.Server
 				foreach (var t in serverTraits.WithInterface<INotifySyncLobbyInfo>())
 					t.LobbyInfoSynced(this);
 			}
+		}
 
 		public void SyncLobbyGlobalSettings()
 		{
 			if (State != ServerState.WaitingPlayers)
 				return;
 
+			lock (LobbyInfo)
+			{
 				var sessionData = new List<MiniYamlNode> { LobbyInfo.GlobalSettings.Serialize() };
 
 				DispatchOrders(null, 0, Order.FromTargetString("SyncLobbyGlobalSettings", sessionData.WriteToString(), true).Serialize());
@@ -972,8 +992,11 @@ namespace OpenRA.Server
 				foreach (var t in serverTraits.WithInterface<INotifySyncLobbyInfo>())
 					t.LobbyInfoSynced(this);
 			}
+		}
 
 		public void SyncClientPing()
+		{
+			lock (LobbyInfo)
 			{
 				// TODO: Split this further into per client ping orders
 				var clientPings = LobbyInfo.ClientPings.Select(ping => ping.Serialize()).ToList();
@@ -981,8 +1004,11 @@ namespace OpenRA.Server
 				// Note that syncing pings doesn't trigger INotifySyncLobbyInfo
 				DispatchOrders(null, 0, Order.FromTargetString("SyncClientPings", clientPings.WriteToString(), true).Serialize());
 			}
+		}
 
 		public void StartGame()
+		{
+			lock (LobbyInfo)
 			{
 				foreach (var listener in listeners)
 					listener.Stop();
@@ -1047,6 +1073,7 @@ namespace OpenRA.Server
 					});
 				}
 			}
+		}
 
 		public ConnectionTarget GetEndpointForLocalConnection()
 		{

OpenRA.Mods.Common/ServerTraits/LobbyCommands.cs

@@ -48,6 +48,8 @@ namespace OpenRA.Mods.Common.Server
 		};
 
 		static bool ValidateSlotCommand(S server, Connection conn, Session.Client client, string arg, bool requiresHost)
+		{
+			lock (server.LobbyInfo)
 			{
 				if (!server.LobbyInfo.Slots.ContainsKey(arg))
 				{
@@ -63,8 +65,11 @@ namespace OpenRA.Mods.Common.Server
 
 				return true;
 			}
+		}
 
 		public static bool ValidateCommand(S server, Connection conn, Session.Client client, string cmd)
+		{
+			lock (server.LobbyInfo)
 			{
 				// Kick command is always valid for the host
 				if (cmd.StartsWith("kick "))
@@ -83,6 +88,7 @@ namespace OpenRA.Mods.Common.Server
 
 				return true;
 			}
+		}
 
 		public bool InterpretCommand(S server, Connection conn, Session.Client client, string cmd)
 		{
@@ -99,6 +105,8 @@ namespace OpenRA.Mods.Common.Server
 		}
 
 		static void CheckAutoStart(S server)
+		{
+			lock (server.LobbyInfo)
 			{
 				var nonBotPlayers = server.LobbyInfo.NonBotPlayers;
 
@@ -117,29 +125,31 @@ namespace OpenRA.Mods.Common.Server
 
 				server.StartGame();
 			}
+		}
 
 		static bool State(S server, Connection conn, Session.Client client, string s)
 		{
-			var state = Session.ClientState.Invalid;
+			lock (server.LobbyInfo)
-			if (!Enum<Session.ClientState>.TryParse(s, false, out state))
+			{
+				if (!Enum<Session.ClientState>.TryParse(s, false, out var state))
 				{
 					server.SendOrderTo(conn, "Message", "Malformed state command");
 					return true;
 				}
 
 				client.State = state;
-
+				Log.Write("server", "Player @{0} is {1}", conn.Socket.RemoteEndPoint, client.State);
-			Log.Write("server", "Player @{0} is {1}",
-				conn.Socket.RemoteEndPoint, client.State);
 
 				server.SyncLobbyClients();
-
 				CheckAutoStart(server);
 
 				return true;
 			}
+		}
 
 		static bool StartGame(S server, Connection conn, Session.Client client, string s)
+		{
+			lock (server.LobbyInfo)
 			{
 				if (!client.IsAdmin)
 				{
@@ -161,10 +171,14 @@ namespace OpenRA.Mods.Common.Server
 				}
 
 				server.StartGame();
+
 				return true;
 			}
+		}
 
 		static bool Slot(S server, Connection conn, Session.Client client, string s)
+		{
+			lock (server.LobbyInfo)
 			{
 				if (!server.LobbyInfo.Slots.ContainsKey(s))
 				{
@@ -193,22 +207,27 @@ namespace OpenRA.Mods.Common.Server
 
 				return true;
 			}
+		}
 
 		static bool AllowSpectators(S server, Connection conn, Session.Client client, string s)
+		{
+			lock (server.LobbyInfo)
 			{
 				if (bool.TryParse(s, out server.LobbyInfo.GlobalSettings.AllowSpectators))
 				{
 					server.SyncLobbyGlobalSettings();
 					return true;
 				}
-			else
+
-			{
 				server.SendOrderTo(conn, "Message", "Malformed allow_spectate command");
+
 				return true;
 			}
 		}
 
 		static bool Specate(S server, Connection conn, Session.Client client, string s)
+		{
+			lock (server.LobbyInfo)
 			{
 				if (server.LobbyInfo.GlobalSettings.AllowSpectators || client.IsAdmin)
 				{
@@ -220,11 +239,14 @@ namespace OpenRA.Mods.Common.Server
 					CheckAutoStart(server);
 					return true;
 				}
-			else
+
 				return false;
 			}
+		}
 
 		static bool SlotClose(S server, Connection conn, Session.Client client, string s)
+		{
+			lock (server.LobbyInfo)
 			{
 				if (!ValidateSlotCommand(server, conn, client, s, true))
 					return false;
@@ -260,8 +282,11 @@ namespace OpenRA.Mods.Common.Server
 
 				return true;
 			}
+		}
 
 		static bool SlotOpen(S server, Connection conn, Session.Client client, string s)
+		{
+			lock (server.LobbyInfo)
 			{
 				if (!ValidateSlotCommand(server, conn, client, s, true))
 					return false;
@@ -287,11 +312,13 @@ namespace OpenRA.Mods.Common.Server
 
 				return true;
 			}
+		}
 
 		static bool SlotBot(S server, Connection conn, Session.Client client, string s)
+		{
+			lock (server.LobbyInfo)
 			{
 				var parts = s.Split(' ');
-
 				if (parts.Length < 3)
 				{
 					server.SendOrderTo(conn, "Message", "Malformed slot_bot command");
@@ -366,8 +393,11 @@ namespace OpenRA.Mods.Common.Server
 
 				return true;
 			}
+		}
 
 		static bool Map(S server, Connection conn, Session.Client client, string s)
+		{
+			lock (server.LobbyInfo)
 			{
 				if (!client.IsAdmin)
 				{
@@ -377,6 +407,8 @@ namespace OpenRA.Mods.Common.Server
 
 				var lastMap = server.LobbyInfo.GlobalSettings.Map;
 				Action<MapPreview> selectMap = map =>
+				{
+					lock (server.LobbyInfo)
 					{
 						// Make sure the map hasn't changed in the meantime
 						if (server.LobbyInfo.GlobalSettings.Map != lastMap)
@@ -456,10 +488,10 @@ namespace OpenRA.Mods.Common.Server
 						var briefing = MissionBriefingOrDefault(server);
 						if (briefing != null)
 							server.SendMessage(briefing);
+					}
 				};
 
-			Action queryFailed = () =>
+				Action queryFailed = () => server.SendOrderTo(conn, "Message", "Map was not found on server.");
-				server.SendOrderTo(conn, "Message", "Map was not found on server.");
 
 				var m = server.ModData.MapCache[s];
 				if (m.Status == MapStatus.Available || m.Status == MapStatus.DownloadAvailable)
@@ -475,8 +507,11 @@ namespace OpenRA.Mods.Common.Server
 
 				return true;
 			}
+		}
 
 		static bool Option(S server, Connection conn, Session.Client client, string s)
+		{
+			lock (server.LobbyInfo)
 			{
 				if (!client.IsAdmin)
 				{
@@ -530,8 +565,11 @@ namespace OpenRA.Mods.Common.Server
 
 				return true;
 			}
+		}
 
 		static bool AssignTeams(S server, Connection conn, Session.Client client, string s)
+		{
+			lock (server.LobbyInfo)
 			{
 				if (!client.IsAdmin)
 				{
@@ -570,8 +608,11 @@ namespace OpenRA.Mods.Common.Server
 
 				return true;
 			}
+		}
 
 		static bool Kick(S server, Connection conn, Session.Client client, string s)
+		{
+			lock (server.LobbyInfo)
 			{
 				if (!client.IsAdmin)
 				{
@@ -608,7 +649,6 @@ namespace OpenRA.Mods.Common.Server
 				server.DropClient(kickConn);
 
 				bool.TryParse(split[1], out var tempBan);
-
 				if (tempBan)
 				{
 					Log.Write("server", "Temporarily banning client {0} ({1}).", kickClientID, kickClient.IPAddress);
@@ -621,8 +661,11 @@ namespace OpenRA.Mods.Common.Server
 
 				return true;
 			}
+		}
 
 		static bool MakeAdmin(S server, Connection conn, Session.Client client, string s)
+		{
+			lock (server.LobbyInfo)
 			{
 				if (!client.IsAdmin)
 				{
@@ -655,8 +698,11 @@ namespace OpenRA.Mods.Common.Server
 
 				return true;
 			}
+		}
 
 		static bool MakeSpectator(S server, Connection conn, Session.Client client, string s)
+		{
+			lock (server.LobbyInfo)
 			{
 				if (!client.IsAdmin)
 				{
@@ -686,8 +732,11 @@ namespace OpenRA.Mods.Common.Server
 
 				return true;
 			}
+		}
 
 		static bool Name(S server, Connection conn, Session.Client client, string s)
+		{
+			lock (server.LobbyInfo)
 			{
 				var sanitizedName = Settings.SanitizedPlayerName(s);
 				if (sanitizedName == client.Name)
@@ -700,8 +749,11 @@ namespace OpenRA.Mods.Common.Server
 
 				return true;
 			}
+		}
 
 		static bool Faction(S server, Connection conn, Session.Client client, string s)
+		{
+			lock (server.LobbyInfo)
 			{
 				var parts = s.Split(' ');
 				var targetClient = server.LobbyInfo.ClientWithIndex(Exts.ParseIntegerInvariant(parts[0]));
@@ -729,8 +781,11 @@ namespace OpenRA.Mods.Common.Server
 
 				return true;
 			}
+		}
 
 		static bool Team(S server, Connection conn, Session.Client client, string s)
+		{
+			lock (server.LobbyInfo)
 			{
 				var parts = s.Split(' ');
 				var targetClient = server.LobbyInfo.ClientWithIndex(Exts.ParseIntegerInvariant(parts[0]));
@@ -754,8 +809,11 @@ namespace OpenRA.Mods.Common.Server
 
 				return true;
 			}
+		}
 
 		static bool Spawn(S server, Connection conn, Session.Client client, string s)
+		{
+			lock (server.LobbyInfo)
 			{
 				var parts = s.Split(' ');
 				var targetClient = server.LobbyInfo.ClientWithIndex(Exts.ParseIntegerInvariant(parts[0]));
@@ -806,8 +864,11 @@ namespace OpenRA.Mods.Common.Server
 
 				return true;
 			}
+		}
 
 		static bool PlayerColor(S server, Connection conn, Session.Client client, string s)
+		{
+			lock (server.LobbyInfo)
 			{
 				var parts = s.Split(' ');
 				var targetClient = server.LobbyInfo.ClientWithIndex(Exts.ParseIntegerInvariant(parts[0]));
@@ -832,8 +893,11 @@ namespace OpenRA.Mods.Common.Server
 
 				return true;
 			}
+		}
 
 		static bool SyncLobby(S server, Connection conn, Session.Client client, string s)
+		{
+			lock (server.LobbyInfo)
 			{
 				if (!client.IsAdmin)
 				{
@@ -853,8 +917,11 @@ namespace OpenRA.Mods.Common.Server
 
 				return true;
 			}
+		}
 
 		public void ServerStarted(S server)
+		{
+			lock (server.LobbyInfo)
 			{
 				// Remote maps are not supported for the initial map
 				var uid = server.LobbyInfo.GlobalSettings.Map;
@@ -869,10 +936,13 @@ namespace OpenRA.Mods.Common.Server
 
 				LoadMapSettings(server, server.LobbyInfo.GlobalSettings, server.Map.Rules);
 			}
+		}
 
 		static Session.Slot MakeSlotFromPlayerReference(PlayerReference pr)
 		{
-			if (!pr.Playable) return null;
+			if (!pr.Playable)
+				return null;
+
 			return new Session.Slot
 			{
 				PlayerReference = pr.Name,
@@ -887,6 +957,8 @@ namespace OpenRA.Mods.Common.Server
 		}
 
 		public static void LoadMapSettings(S server, Session.Global gs, Ruleset rules)
+		{
+			lock (server.LobbyInfo)
 			{
 				var options = rules.Actors["player"].TraitInfos<ILobbyOptions>()
 					.Concat(rules.Actors["world"].TraitInfos<ILobbyOptions>())
@@ -925,8 +997,11 @@ namespace OpenRA.Mods.Common.Server
 					}
 				}
 			}
+		}
 
 		static Color SanitizePlayerColor(S server, Color askedColor, int playerIndex, Connection connectionToEcho = null)
+		{
+			lock (server.LobbyInfo)
 			{
 				var validator = server.ModData.Manifest.Get<ColorValidator>();
 				var askColor = askedColor;
@@ -944,6 +1019,7 @@ namespace OpenRA.Mods.Common.Server
 
 				return validator.MakeValid(askColor, server.Random, terrainColors, playerColors, onError);
 			}
+		}
 
 		static string MissionBriefingOrDefault(S server)
 		{
@@ -955,6 +1031,8 @@ namespace OpenRA.Mods.Common.Server
 		}
 
 		public void ClientJoined(S server, Connection conn)
+		{
+			lock (server.LobbyInfo)
 			{
 				var client = server.GetClient(conn);
 
@@ -969,8 +1047,11 @@ namespace OpenRA.Mods.Common.Server
 				if (briefing != null)
 					server.SendOrderTo(conn, "Message", briefing);
 			}
+		}
 
 		void INotifyServerEmpty.ServerEmpty(S server)
+		{
+			lock (server.LobbyInfo)
 			{
 				// Expire any temporary bans
 				server.TempBans.Clear();
@@ -984,6 +1065,7 @@ namespace OpenRA.Mods.Common.Server
 					.Where(ss => ss != null)
 					.ToDictionary(ss => ss.PlayerReference, ss => ss);
 			}
+		}
 
 		public static PlayerReference PlayerReferenceForSlot(S server, Session.Slot slot)
 		{

OpenRA.Mods.Common/ServerTraits/LobbySettingsNotification.cs

@@ -20,6 +20,8 @@ namespace OpenRA.Mods.Common.Server
 	public class LobbySettingsNotification : ServerTrait, IClientJoined
 	{
 		public void ClientJoined(OpenRA.Server.Server server, Connection conn)
+		{
+			lock (server.LobbyInfo)
 			{
 				if (server.LobbyInfo.ClientWithIndex(conn.PlayerIndex).IsAdmin)
 					return;
@@ -43,4 +45,5 @@ namespace OpenRA.Mods.Common.Server
 				}
 			}
 		}
+	}
 }

OpenRA.Mods.Common/ServerTraits/PlayerPinger.cs

@@ -36,7 +36,11 @@ namespace OpenRA.Mods.Common.Server
 				lastPing = Game.RunTime;
 
 				// Ignore client timeout in singleplayer games to make debugging easier
-				if (server.LobbyInfo.NonBotClients.Count() < 2 && server.Type != ServerType.Dedicated)
+				var nonBotClientCount = 0;
+				lock (server.LobbyInfo)
+					nonBotClientCount = server.LobbyInfo.NonBotClients.Count();
+
+				if (nonBotClientCount < 2 && server.Type != ServerType.Dedicated)
 					foreach (var c in server.Conns.ToList())
 						server.SendOrderTo(c, "Ping", Game.RunTime.ToString());
 				else