From 7a64527db5b23c8b9d988f1d914fbd3555025830 Mon Sep 17 00:00:00 2001
From: cx384 <cx384@proton.me>
Date: Sat, 15 Jun 2024 16:00:33 +0200
Subject: [PATCH] Fix connected_players on_shutdown (#14739)

---
 doc/lua_api.md                               |  2 +
 games/devtest/mods/unittests/init.lua        |  1 +
 games/devtest/mods/unittests/on_shutdown.lua | 22 ++++++++++
 src/server.cpp                               | 46 +++++++++-----------
 4 files changed, 46 insertions(+), 25 deletions(-)
 create mode 100644 games/devtest/mods/unittests/on_shutdown.lua

diff --git a/doc/lua_api.md b/doc/lua_api.md
index b1412ca85..c733fc0a3 100644
--- a/doc/lua_api.md
+++ b/doc/lua_api.md
@@ -5720,6 +5720,8 @@ Call these functions only at load time!
       aliases handled.
 * `minetest.register_on_shutdown(function())`
     * Called before server shutdown
+    * Players that were kicked by the shutdown procedure are still fully accessible
+     in `minetest.get_connected_players()`.
     * **Warning**: If the server terminates abnormally (i.e. crashes), the
       registered callbacks **will likely not be run**. Data should be saved at
       semi-frequent intervals as well as on server shutdown.
diff --git a/games/devtest/mods/unittests/init.lua b/games/devtest/mods/unittests/init.lua
index 47568d9fc..eae003a2a 100644
--- a/games/devtest/mods/unittests/init.lua
+++ b/games/devtest/mods/unittests/init.lua
@@ -186,6 +186,7 @@ dofile(modpath .. "/metadata.lua")
 dofile(modpath .. "/raycast.lua")
 dofile(modpath .. "/inventory.lua")
 dofile(modpath .. "/load_time.lua")
+dofile(modpath .. "/on_shutdown.lua")
 
 --------------
 
diff --git a/games/devtest/mods/unittests/on_shutdown.lua b/games/devtest/mods/unittests/on_shutdown.lua
new file mode 100644
index 000000000..6d5d88638
--- /dev/null
+++ b/games/devtest/mods/unittests/on_shutdown.lua
@@ -0,0 +1,22 @@
+-- Test whether players still exist on shutdown
+local players = {}
+
+core.register_on_joinplayer(function(player)
+	players[player:get_player_name()] = true
+end)
+
+core.register_on_leaveplayer(function(player)
+	local name = player:get_player_name();
+	assert(players[name], "Unrecorded player join.")
+	players[name] = nil
+end)
+
+core.register_on_shutdown(function()
+	for _, player in pairs(core.get_connected_players()) do
+		local name = player:get_player_name()
+		assert(players[name], "Unrecorded player join or left too early.")
+		players[name] = nil
+	end
+
+	assert(not next(players), "Invalid connected players on shutdown.")
+end)
diff --git a/src/server.cpp b/src/server.cpp
index 316f349b2..937cbe90a 100644
--- a/src/server.cpp
+++ b/src/server.cpp
@@ -329,27 +329,6 @@ Server::~Server()
 	SendChatMessage(PEER_ID_INEXISTENT, ChatMessage(CHATMESSAGE_TYPE_ANNOUNCE,
 			L"*** Server shutting down"));
 
-	if (m_env) {
-		MutexAutoLock envlock(m_env_mutex);
-
-		infostream << "Server: Saving players" << std::endl;
-		m_env->saveLoadedPlayers();
-
-		infostream << "Server: Kicking players" << std::endl;
-		std::string kick_msg;
-		bool reconnect = false;
-		if (isShutdownRequested()) {
-			reconnect = m_shutdown_state.should_reconnect;
-			kick_msg = m_shutdown_state.message;
-		}
-		if (kick_msg.empty()) {
-			kick_msg = g_settings->get("kick_msg_shutdown");
-		}
-		m_env->saveLoadedPlayers(true);
-		kickAllPlayers(SERVER_ACCESSDENIED_SHUTDOWN,
-			kick_msg, reconnect);
-	}
-
 	actionstream << "Server: Shutting down" << std::endl;
 
 	// Stop server step from happening
@@ -369,16 +348,33 @@ Server::~Server()
 	if (m_env) {
 		MutexAutoLock envlock(m_env_mutex);
 
+		infostream << "Server: Executing shutdown hooks" << std::endl;
 		try {
-			// Empty out the environment, this can also invoke callbacks.
-			m_env->deactivateBlocksAndObjects();
+			m_script->on_shutdown();
 		} catch (ModError &e) {
 			addShutdownError(e);
 		}
 
-		infostream << "Server: Executing shutdown hooks" << std::endl;
+		infostream << "Server: Saving players" << std::endl;
+		m_env->saveLoadedPlayers();
+
+		infostream << "Server: Kicking players" << std::endl;
+		std::string kick_msg;
+		bool reconnect = false;
+		if (isShutdownRequested()) {
+			reconnect = m_shutdown_state.should_reconnect;
+			kick_msg = m_shutdown_state.message;
+		}
+		if (kick_msg.empty()) {
+			kick_msg = g_settings->get("kick_msg_shutdown");
+		}
+		m_env->saveLoadedPlayers(true);
+		kickAllPlayers(SERVER_ACCESSDENIED_SHUTDOWN,
+			kick_msg, reconnect);
+
 		try {
-			m_script->on_shutdown();
+			// Empty out the environment, this can also invoke callbacks.
+			m_env->deactivateBlocksAndObjects();
 		} catch (ModError &e) {
 			addShutdownError(e);
 		}