Merge pull request #397 from stevenh/general-devel

Refactored server stop and world save operations fixing race condition

TShockAPI/BackupManager.cs

@@ -63,11 +63,7 @@ namespace TShockAPI
 				TShock.Utils.Broadcast("Server map saving, potential lag spike");
 				Console.WriteLine("Backing up world...");
 
-				Thread SaveWorld = new Thread(TShock.Utils.SaveWorld);
+				SaveManager.Instance.SaveWorld();
-				SaveWorld.Start();
-
-				while (SaveWorld.ThreadState == ThreadState.Running)
-					Thread.Sleep(50);
 				Console.WriteLine("World backed up");
 				Console.ForegroundColor = ConsoleColor.Gray;
 				Log.Info(string.Format("World backed up ({0})", Main.worldPathName));

TShockAPI/Commands.cs

@@ -1007,38 +1007,37 @@ namespace TShockAPI
 				}
 			}
 
-			TShock.Utils.ForceKickAll("Server shutting down!");
+			TShock.Utils.StopServer();
-			WorldGen.saveWorld();
-			Netplay.disconnect = true;
 		}
-        //Added restart command
+		//Added restart command
-        private static void Restart(CommandArgs args)
+		private static void Restart(CommandArgs args)
-        {
+		{
-	if (Main.runningMono){
+			if (Main.runningMono)
-	Log.ConsoleInfo("Sorry, this command has not yet been implemented in Mono");
+			{
-	}else{
+				Log.ConsoleInfo("Sorry, this command has not yet been implemented in Mono");
-            if (TShock.Config.ServerSideInventory)
+			}
-            {
+			else
-                foreach (TSPlayer player in TShock.Players)
+			{
-                {
+				if (TShock.Config.ServerSideInventory)
-                    if (player != null && player.IsLoggedIn && !player.IgnoreActionsForClearingTrashCan)
+				{
-                    {
+					foreach (TSPlayer player in TShock.Players)
-                        TShock.InventoryDB.InsertPlayerData(player);
+					{
-                    }
+						if (player != null && player.IsLoggedIn && !player.IgnoreActionsForClearingTrashCan)
-                }
+						{
-            }
+							TShock.InventoryDB.InsertPlayerData(player);
+						}
+					}
+				}
 
-            TShock.Utils.ForceKickAll("Server restarting!");
+				TShock.Utils.StopServer();
-            WorldGen.saveWorld();
+				System.Diagnostics.Process.Start(System.Reflection.Assembly.GetExecutingAssembly().GetName().CodeBase);
-            Netplay.disconnect = true;
+				Environment.Exit(0);
-            System.Diagnostics.Process.Start(System.Reflection.Assembly.GetExecutingAssembly().GetName().CodeBase);
+			}
-            Environment.Exit(0);
+		}
-        }}
 
 		private static void OffNoSave(CommandArgs args)
 		{
-			TShock.Utils.ForceKickAll("Server shutting down!");
+			TShock.Utils.StopServer(false);
-			Netplay.disconnect = true;
 		}
 
 		private static void CheckUpdates(CommandArgs args)
@@ -2258,10 +2257,7 @@ namespace TShockAPI
 		{
 			Main.spawnTileX = args.Player.TileX + 1;
 			Main.spawnTileY = args.Player.TileY + 3;
-
+			SaveManager.Instance.SaveWorld(false);
-			TShock.Utils.Broadcast("Server map saving, potential lag spike");
-			Thread SaveWorld = new Thread(TShock.Utils.SaveWorld);
-			SaveWorld.Start();
 		}
 
 		private static void Reload(CommandArgs args)
@@ -2288,9 +2284,7 @@ namespace TShockAPI
 
 		private static void Save(CommandArgs args)
 		{
-			TShock.Utils.Broadcast("Server map saving, potential lag spike");
+			SaveManager.Instance.SaveWorld(false);
-			Thread SaveWorld = new Thread(TShock.Utils.SaveWorld);
-			SaveWorld.Start();
 		}
 
 		private static void Settle(CommandArgs args)

TShockAPI/RconHandler.cs

@@ -253,9 +253,7 @@ namespace TShockAPI
 				WorldGen.genRand = new Random();
 			if (text.StartsWith("exit"))
 			{
-				TShock.Utils.ForceKickAll("Server shutting down!");
+				TShock.Utils.StopServer();
-				WorldGen.saveWorld(false);
-				Netplay.disconnect = true;
 				return "Server shutting down.";
 			}
 			else if (text.StartsWith("playing") || text.StartsWith("/playing"))

TShockAPI/Rest/RestManager.cs

@@ -102,16 +102,10 @@ namespace TShockAPI
 			if (!GetBool(parameters["confirm"], false))
 				return RestInvalidParam("confirm");
 
-			if (!GetBool(parameters["nosave"], false))
-				WorldGen.saveWorld();
-			Netplay.disconnect = true;
-
 			// Inform players the server is shutting down
 			var msg = string.IsNullOrWhiteSpace(parameters["message"]) ? "Server is shutting down" : parameters["message"];
-			foreach (TSPlayer player in TShock.Players.Where(p => null != p))
+			TShock.Utils.StopServer(!GetBool(parameters["nosave"], false), msg);
-			{
+
-				TShock.Utils.ForceKick(player, msg);
-			}
 			return RestResponse("The server is shutting down");
 		}
 
@@ -304,8 +298,8 @@ namespace TShockAPI
 			if (ret is RestObject)
 				return ret;
 
-			User user = (User)ret;
+			User user = (User)ret;
-			return new RestObject() { { "group", user.Group }, { "id", user.ID.ToString() } };
+			return new RestObject() { { "group", user.Group }, { "id", user.ID.ToString() }, { "name", user.Name } };
 		}
 
 		#endregion
@@ -411,7 +405,7 @@ namespace TShockAPI
 
 		private object WorldSave(RestVerbs verbs, IParameterCollection parameters)
 		{
-			TShock.Utils.SaveWorld();
+			SaveManager.Instance.SaveWorld();
 
 			return RestResponse("World saved");
 		}

TShockAPI/SaveManager.cs

@@ -0,0 +1,123 @@
+using System;
+using System.Collections.Generic;
+using System.ComponentModel;
+using System.Threading;
+using System.Diagnostics;
+using Terraria;
+
+namespace TShockAPI
+{
+	class SaveManager : IDisposable
+	{
+		// Singleton
+		private static readonly SaveManager instance = new SaveManager();
+		private SaveManager() 
+		{
+			_saveThread = new Thread(SaveWorker);
+			_saveThread.Name = "TShock SaveManager Worker";
+			_saveThread.Start();
+		}
+		public static SaveManager Instance { get { return instance; } }
+
+		// Producer Consumer
+		private EventWaitHandle _wh = new AutoResetEvent(false);
+		private Object _saveLock = new Object();
+		private Queue<SaveTask> _saveQueue = new Queue<SaveTask>();
+		private Thread _saveThread;
+		private int saveQueueCount { get { lock (_saveLock) return _saveQueue.Count; } }
+
+		/// <summary>
+		/// SaveWorld event handler which notifies users that the server may lag
+		/// </summary>
+		public void OnSaveWorld(bool resettime = false, HandledEventArgs e = null)
+		{
+			TShock.Utils.Broadcast("Saving world. Momentary lag might result from this.", Color.Red);
+		}
+
+		/// <summary>
+		/// Saves the map data
+		/// </summary>
+		/// <param name="wait">wait for all pending saves to finish (default: true)</param>
+		/// <param name="resetTime">reset the last save time counter (default: false)</param>
+		/// <param name="direct">use the realsaveWorld method instead of saveWorld event (default: false)</param>
+		public void SaveWorld(bool wait = true, bool resetTime = false, bool direct = false)
+		{
+			EnqueueTask(new SaveTask(resetTime, direct));
+			if (!wait)
+				return;
+
+			// Wait for all outstanding saves to complete
+			int count = saveQueueCount;
+			while (0 != count)
+			{
+				Thread.Sleep(50);
+				count = saveQueueCount;
+			}
+		}
+
+		/// <summary>
+		/// Processes any outstanding saves, shutsdown the save thread and returns
+		/// </summary>
+		public void Dispose()
+		{
+			EnqueueTask(null);
+			_saveThread.Join();
+			_wh.Close();
+		}
+
+		private void EnqueueTask(SaveTask task)
+		{
+			lock (_saveLock)
+			{
+				_saveQueue.Enqueue(task);
+			}
+			_wh.Set();
+		}
+
+		private void SaveWorker()
+		{
+			while (true)
+			{
+				lock (_saveLock)
+				{
+					// NOTE: lock for the entire process so wait works in SaveWorld
+					if (_saveQueue.Count > 0)
+					{
+						SaveTask task = _saveQueue.Dequeue();
+						if (null == task)
+							return;
+						else
+						{
+							if (task.direct)
+							{
+								OnSaveWorld();
+								WorldGen.realsaveWorld(task.resetTime);
+							}
+							else
+								WorldGen.saveWorld(task.resetTime);
+							TShock.Utils.Broadcast("World saved.", Color.Yellow);
+							Log.Info(string.Format("World saved at ({0})", Main.worldPathName));
+						}
+					}
+				}
+				_wh.WaitOne();
+			}
+		}
+
+		class SaveTask
+		{
+			public bool resetTime { get; set; }
+			public bool direct { get; set; }
+			public SaveTask(bool resetTime, bool direct)
+			{
+				this.resetTime = resetTime;
+				this.direct = direct;
+			}
+		
+			public override string ToString()
+			{
+				return string.Format("resetTime {0}, direct {1}", resetTime, direct);
+			}
+		}
+	}
+}

TShockAPI/TShock.cs

@@ -62,7 +62,7 @@ namespace TShockAPI
 		public static GeoIPCountry Geo;
 		public static SecureRest RestApi;
 		public static RestManager RestManager;
-		public static Utils Utils = new Utils();
+		public static Utils Utils = Utils.Instance;
 		public static StatTracker StatTracker = new StatTracker();
 		/// <summary>
 		/// Used for implementing REST Tokens prior to the REST system starting up.
@@ -186,7 +186,6 @@ namespace TShockAPI
 				if (Config.EnableGeoIP && File.Exists(geoippath))
 					Geo = new GeoIPCountry(geoippath);
 
-				Console.Title = string.Format("TerrariaShock Version {0} ({1})", Version, VersionCodename);
 				Log.ConsoleInfo(string.Format("TerrariaShock Version {0} ({1}) now running.", Version, VersionCodename));
 
 				GameHooks.PostInitialize += OnPostInit;
@@ -203,7 +202,7 @@ namespace TShockAPI
 			    NpcHooks.SetDefaultsInt += OnNpcSetDefaults;
 				ProjectileHooks.SetDefaults += OnProjectileSetDefaults;
 				WorldHooks.StartHardMode += OnStartHardMode;
-                WorldHooks.SaveWorld += OnSaveWorld;
+				WorldHooks.SaveWorld += SaveManager.Instance.OnSaveWorld;
 
 				GetDataHandlers.InitGetDataHandler();
 				Commands.InitCommands();
@@ -260,10 +259,13 @@ namespace TShockAPI
 		{
 			if (disposing)
 			{
+				// NOTE: order is important here
 				if (Geo != null)
 				{
 					Geo.Dispose();
 				}
+				SaveManager.Instance.Dispose();
+
 				GameHooks.PostInitialize -= OnPostInit;
 				GameHooks.Update -= OnUpdate;
                 ServerHooks.Connect -= OnConnect;
@@ -278,15 +280,16 @@ namespace TShockAPI
                 NpcHooks.SetDefaultsInt -= OnNpcSetDefaults;
 				ProjectileHooks.SetDefaults -= OnProjectileSetDefaults;
                 WorldHooks.StartHardMode -= OnStartHardMode;
-                WorldHooks.SaveWorld -= OnSaveWorld;
+				WorldHooks.SaveWorld -= SaveManager.Instance.OnSaveWorld;
+
 				if (File.Exists(Path.Combine(SavePath, "tshock.pid")))
 				{
 					File.Delete(Path.Combine(SavePath, "tshock.pid"));
 				}
+
 				RestApi.Dispose();
 				Log.Dispose();
 			}
-
 			base.Dispose(disposing);
 		}
 
@@ -322,7 +325,7 @@ namespace TShockAPI
 				if (Main.worldPathName != null && Config.SaveWorldOnCrash)
 				{
 					Main.worldPathName += ".crash";
-					WorldGen.saveWorld();
+					SaveManager.Instance.SaveWorld();
 				}
 			}
 		}
@@ -361,36 +364,33 @@ namespace TShockAPI
 		{
 			for (int i = 0; i < parms.Length; i++)
 			{
-				if (parms[i].ToLower() == "-port")
+				switch(parms[i].ToLower())
 				{
-					int port = Convert.ToInt32(parms[++i]);
+					case "-port":
-					Netplay.serverPort = port;
+						int port = Convert.ToInt32(parms[++i]);
-					Config.ServerPort = port;
+						Netplay.serverPort = port;
-					OverridePort = true;
+						Config.ServerPort = port;
-					Log.ConsoleInfo("Port overridden by startup argument. Set to " + port);
+						OverridePort = true;
-				}
+						Log.ConsoleInfo("Port overridden by startup argument. Set to " + port);
-				if (parms[i].ToLower() == "-rest-token")
+						break;
-				{
+					case "-rest-token":
-					string token = Convert.ToString(parms[++i]);
+						string token = Convert.ToString(parms[++i]);
-					RESTStartupTokens.Add(token, "null");
+						RESTStartupTokens.Add(token, "null");
-					Console.WriteLine("Startup parameter overrode REST token.");
+						Console.WriteLine("Startup parameter overrode REST token.");
-				}
+						break;
-				if (parms[i].ToLower() == "-rest-enabled")
+					case "-rest-enabled":
-				{
+						Config.RestApiEnabled = Convert.ToBoolean(parms[++i]);
-					Config.RestApiEnabled = Convert.ToBoolean(parms[++i]);
+						Console.WriteLine("Startup parameter overrode REST enable.");
-					Console.WriteLine("Startup parameter overrode REST enable.");
+						break;
-
+					case "-rest-port":
-				}
+						Config.RestApiPort = Convert.ToInt32(parms[++i]);
-				if (parms[i].ToLower() == "-rest-port")
+						Console.WriteLine("Startup parameter overrode REST port.");
-				{
+						break;
-					Config.RestApiPort = Convert.ToInt32(parms[++i]);
+					case "-maxplayers":
-					Console.WriteLine("Startup parameter overrode REST port.");
+					case "-players":
-
+						Config.MaxSlots = Convert.ToInt32(parms[++i]);
-				}
+						Console.WriteLine("Startup parameter overrode maximum player slot configuration value.");
-				if ((parms[i].ToLower() == "-maxplayers")||(parms[i].ToLower() == "-players"))
+						break;
-				{
-					Config.MaxSlots = Convert.ToInt32(parms[++i]);
-					Console.WriteLine("Startup parameter overrode maximum player slot configuration value.");
 				}
 			}
 		}
@@ -404,6 +404,7 @@ namespace TShockAPI
 
 		private void OnPostInit()
 		{
+			SetConsoleTitle();
 			if (!File.Exists(Path.Combine(SavePath, "auth.lck")) && !File.Exists(Path.Combine(SavePath, "authcode.txt")))
 			{
 				var r = new Random((int) DateTime.Now.ToBinary());
@@ -465,7 +466,6 @@ namespace TShockAPI
 			StatTracker.CheckIn();
 			if (Backups.IsBackupTime)
 				Backups.Backup();
-
 			//call these every second, not every update
 			if ((DateTime.UtcNow - LastCheck).TotalSeconds >= 1)
 			{
@@ -590,8 +590,13 @@ namespace TShockAPI
 					}
 				}
 			}
-			Console.Title = string.Format("TerrariaShock Version {0} ({1}) ({2}/{3})", Version, VersionCodename, count,
+			SetConsoleTitle();
-										  Config.MaxSlots);
+		}
+
+		private void SetConsoleTitle()
+		{
+			Console.Title = string.Format("{0} - {1}/{2} @ {3}:{4} (TerrariaShock v{5})", Config.ServerName, Utils.ActivePlayers(),
+								  Config.MaxSlots, Netplay.serverListenIP, Config.ServerPort, Version);
 		}
 
 		private void OnConnect(int ply, HandledEventArgs handler)
@@ -1012,17 +1017,6 @@ namespace TShockAPI
 				e.Handled = true;
 		}
 
-        void OnSaveWorld(bool resettime, HandledEventArgs e)
-        {
-            if (!Utils.saving)
-            {
-                Utils.Broadcast("Saving world. Momentary lag might result from this.", Color.Red);
-                var SaveWorld = new Thread(Utils.SaveWorld);
-                SaveWorld.Start();
-            }
-            e.Handled = true;
-        }
-
 	    /*
 		 * Useful stuff:
 		 * */

TShockAPI/Utils.cs

@@ -29,11 +29,10 @@ namespace TShockAPI
 {
 	public class Utils
 	{
-	    public static bool saving = false;
+		// Utils is a Singleton
-
+		private static readonly Utils instance = new Utils();
-		public Utils()
+		private Utils() {}
-		{
+		public static Utils Instance { get { return instance; } }
-		}
 
 		public Random Random = new Random();
 		//private static List<Group> groups = new List<Group>();
@@ -135,11 +134,7 @@ namespace TShockAPI
 		/// </summary>
 		public void SaveWorld()
 		{
-            saving = true;
+			SaveManager.Instance.SaveWorld();
-            WorldGen.realsaveWorld();
-            Broadcast("World saved.", Color.Yellow);
-            Log.Info(string.Format("World saved at ({0})", Main.worldPathName));
-            saving = false;
 		}
 
 		/// <summary>
@@ -186,15 +181,7 @@ namespace TShockAPI
 		/// <returns>int playerCount</returns>
 		public int ActivePlayers()
 		{
-			int num = 0;
+			return Main.player.Where(p => null != p && p.active).Count();
-			foreach (TSPlayer player in TShock.Players)
-			{
-				if (player != null && player.Active)
-				{
-					num++;
-				}
-			}
-			return num;
 		}
 
 		/// <summary>
@@ -510,6 +497,27 @@ namespace TShockAPI
 			}
 		}
 
+		/// <summary>
+		/// Stops the server after kicking all players with a reason message, and optionally saving the world
+		/// </summary>
+		/// <param name="save">bool perform a world save before stop (default: true)</param>
+		/// <param name="reason">string reason (default: "Server shutting down!")</param>
+		public void StopServer(bool save = true, string reason = "Server shutting down!")
+		{
+			ForceKickAll(reason);
+			if (save)
+				SaveManager.Instance.SaveWorld();
+
+			// Save takes a while so kick again
+			ForceKickAll(reason);
+
+			// Broadcast so console can see we are shutting down as well
+			TShock.Utils.Broadcast(reason, Color.Red);
+
+			// Disconnect after kick as that signifies server is exiting and could cause a race
+			Netplay.disconnect = true;
+		}
+
 		/// <summary>
 		/// Kicks a player from the server without checking for immunetokick permission.
 		/// </summary>