From ef0009aea7e67638e9aea36fc406fc3f1190953f Mon Sep 17 00:00:00 2001
From: sfan5 <sfan5@live.de>
Date: Fri, 1 Mar 2024 20:02:58 +0100
Subject: [PATCH] Sort out server destruction order

fixes #14421
---
 src/map.cpp               |  4 ++--
 src/server.cpp            | 50 ++++++++++++++++++++++-----------------
 src/serverenvironment.cpp | 14 +++++------
 src/serverenvironment.h   |  3 +++
 4 files changed, 40 insertions(+), 31 deletions(-)

diff --git a/src/map.cpp b/src/map.cpp
index 9d3cc8376..22731f120 100644
--- a/src/map.cpp
+++ b/src/map.cpp
@@ -1328,8 +1328,8 @@ ServerMap::~ServerMap()
 	}
 	catch(std::exception &e)
 	{
-		infostream<<"ServerMap: Failed to save map to "<<m_savedir
-				<<", exception: "<<e.what()<<std::endl;
+		errorstream << "ServerMap: Failed to save map to " << m_savedir
+				 << ", exception: " << e.what() << std::endl;
 	}
 
 	/*
diff --git a/src/server.cpp b/src/server.cpp
index 8cf3d37ee..7344784cd 100644
--- a/src/server.cpp
+++ b/src/server.cpp
@@ -321,7 +321,6 @@ Server::Server(
 
 Server::~Server()
 {
-
 	// Send shutdown message
 	SendChatMessage(PEER_ID_INEXISTENT, ChatMessage(CHATMESSAGE_TYPE_ANNOUNCE,
 			L"*** Server shutting down"));
@@ -349,18 +348,31 @@ Server::~Server()
 
 	actionstream << "Server: Shutting down" << std::endl;
 
-	// Do this before stopping the server in case mapgen callbacks need to access
-	// server-controlled resources (like ModStorages). Also do them before
-	// shutdown callbacks since they may modify state that is finalized in a
+	// Stop server step from happening
+	if (m_thread) {
+		stop();
+		delete m_thread;
+	}
+
+	// Stop all emerge activity and finish off mapgen callbacks. Do this before
+	// shutdown callbacks since there may be state that is finalized in a
 	// callback.
+	// Note: The emerge manager is not deleted yet because further code can
+	//       still interact with map loading.
 	if (m_emerge)
 		m_emerge->stopThreads();
 
 	if (m_env) {
 		MutexAutoLock envlock(m_env_mutex);
 
-		// Execute script shutdown hooks
-		infostream << "Executing shutdown hooks" << std::endl;
+		try {
+			// Empty out the environment, this can also invoke callbacks.
+			m_env->deactivateBlocksAndObjects();
+		} catch (ModError &e) {
+			addShutdownError(e);
+		}
+
+		infostream << "Server: Executing shutdown hooks" << std::endl;
 		try {
 			m_script->on_shutdown();
 		} catch (ModError &e) {
@@ -369,12 +381,10 @@ Server::~Server()
 
 		infostream << "Server: Saving environment metadata" << std::endl;
 		m_env->saveMeta();
-	}
 
-	// Stop threads
-	if (m_thread) {
-		stop();
-		delete m_thread;
+		// Note that this also deletes and saves the map.
+		delete m_env;
+		m_env = nullptr;
 	}
 
 	// Write any changes before deletion.
@@ -388,22 +398,18 @@ Server::~Server()
 		}
 	}
 
-	// Delete things in the reverse order of creation
+	// Delete the rest in the reverse order of creation
+	delete m_game_settings;
 	delete m_emerge;
-	delete m_env;
-	delete m_rollback;
-	delete m_mod_storage_database;
 	delete m_banmanager;
+	delete m_mod_storage_database;
+	delete m_startup_server_map; // if available
+	delete m_script;
+	delete m_rollback;
 	delete m_itemdef;
 	delete m_nodedef;
 	delete m_craftdef;
 
-	// Deinitialize scripting
-	infostream << "Server: Deinitializing scripting" << std::endl;
-	delete m_script;
-	delete m_startup_server_map; // if available
-	delete m_game_settings;
-
 	while (!m_unsent_map_edit_queue.empty()) {
 		delete m_unsent_map_edit_queue.front();
 		m_unsent_map_edit_queue.pop();
@@ -582,7 +588,7 @@ void Server::start()
 
 void Server::stop()
 {
-	infostream<<"Server: Stopping and waiting threads"<<std::endl;
+	infostream<<"Server: Stopping and waiting for threads"<<std::endl;
 
 	// Stop threads (set run=false first so both start stopping)
 	m_thread->stop();
diff --git a/src/serverenvironment.cpp b/src/serverenvironment.cpp
index df5fb5e12..47fff1ef4 100644
--- a/src/serverenvironment.cpp
+++ b/src/serverenvironment.cpp
@@ -534,18 +534,18 @@ void ServerEnvironment::init()
 	}
 }
 
-ServerEnvironment::~ServerEnvironment()
+void ServerEnvironment::deactivateBlocksAndObjects()
 {
 	// Clear active block list.
 	// This makes the next one delete all active objects.
 	m_active_blocks.clear();
 
-	try {
-		// Convert all objects to static and delete the active objects
-		deactivateFarObjects(true);
-	} catch (ModError &e) {
-		m_server->addShutdownError(e);
-	}
+	deactivateFarObjects(true);
+}
+
+ServerEnvironment::~ServerEnvironment()
+{
+	assert(m_active_blocks.size() == 0); // deactivateBlocksAndObjects does this
 
 	// Drop/delete map
 	if (m_map)
diff --git a/src/serverenvironment.h b/src/serverenvironment.h
index 000f5f81f..f166802a0 100644
--- a/src/serverenvironment.h
+++ b/src/serverenvironment.h
@@ -363,6 +363,9 @@ public:
 	// Clear objects, loading and going through every MapBlock
 	void clearObjects(ClearObjectsMode mode);
 
+	// to be called before destructor
+	void deactivateBlocksAndObjects();
+
 	// This makes stuff happen
 	void step(f32 dtime);