Simplified and fixed server restart procedure:

- Replaced several assertions with runtime_error's to detect them in
release builds
- Removed multiple dispatchMainThread calls in server shutdown code to
simplify debugging and code flow
- Moved handling of gameplay shutdown and score calculation from
PlayerInterface to ServerHandler (not perfect, but better than before)

client/CPlayerInterface.cpp

@@ -1561,31 +1561,6 @@ void CPlayerInterface::gameOver(PlayerColor player, const EVictoryLossCheckResul
 
 		GH.curInt = previousInterface;
 		LOCPLINT = previousInterface;
-
-		if(CSH->howManyPlayerInterfaces() == 1 && !settings["session"]["spectate"].Bool()) //all human players eliminated
-		{
-			if(adventureInt)
-			{
-				GH.windows().popWindows(GH.windows().count());
-				adventureInt.reset();
-			}
-		}
-
-		if (victoryLossCheckResult.victory() && LOCPLINT == this)
-		{
-			// end game if current human player has won
-			CSH->sendClientDisconnecting();
-			requestReturningToMainMenu(true);
-		}
-		else if(CSH->howManyPlayerInterfaces() == 1 && !settings["session"]["spectate"].Bool())
-		{
-			//all human players eliminated
-			CSH->sendClientDisconnecting();
-			requestReturningToMainMenu(false);
-		}
-
-		if (GH.curInt == this)
-			GH.curInt = nullptr;
 	}
 }
 
@@ -1739,50 +1714,6 @@ void CPlayerInterface::showShipyardDialogOrProblemPopup(const IShipyard *obj)
 		showShipyardDialog(obj);
 }
 
-void CPlayerInterface::requestReturningToMainMenu(bool won)
-{
-	HighScoreParameter param;
-	param.difficulty = cb->getStartInfo()->difficulty;
-	param.day = cb->getDate();
-	param.townAmount = cb->howManyTowns();
-	param.usedCheat = cb->getPlayerState(*cb->getPlayerID())->cheated;
-	param.hasGrail = false;
-	for(const CGHeroInstance * h : cb->getHeroesInfo())
-		if(h->hasArt(ArtifactID::GRAIL))
-			param.hasGrail = true;
-	for(const CGTownInstance * t : cb->getTownsInfo())
-		if(t->builtBuildings.count(BuildingID::GRAIL))
-			param.hasGrail = true;
-	param.allDefeated = true;
-	for (PlayerColor player(0); player < PlayerColor::PLAYER_LIMIT; ++player)
-	{
-		auto ps = cb->getPlayerState(player, false);
-		if(ps && player != *cb->getPlayerID())
-			if(!ps->checkVanquished())
-				param.allDefeated = false;
-	}
-	param.scenarioName = cb->getMapHeader()->name.toString();
-	param.playerName = cb->getStartInfo()->playerInfos.find(*cb->getPlayerID())->second.name;
-	HighScoreCalculation highScoreCalc;
-	highScoreCalc.parameters.push_back(param);
-	highScoreCalc.isCampaign = false;
-
-	if(won && cb->getStartInfo()->campState)
-		CSH->startCampaignScenario(param, cb->getStartInfo()->campState);
-	else
-	{
-		GH.dispatchMainThread(
-			[won, highScoreCalc]()
-			{
-				CSH->endGameplay();
-				GH.defActionsDef = 63;
-				CMM->menu->switchToTab("main");
-				GH.windows().createAndPushWindow<CHighScoreInputScreen>(won, highScoreCalc);
-			}
-		);
-	}
-}
-
 void CPlayerInterface::askToAssembleArtifact(const ArtifactLocation &al)
 {
 	if(auto hero = cb->getHero(al.artHolder))

client/CServerHandler.cpp

@@ -34,7 +34,9 @@
 #include "../lib/TurnTimerInfo.h"
 #include "../lib/VCMIDirs.h"
 #include "../lib/campaign/CampaignState.h"
+#include "../lib/CPlayerState.h"
 #include "../lib/mapping/CMapInfo.h"
+#include "../lib/mapObjects/CGTownInstance.h"
 #include "../lib/mapObjects/MiscObjects.h"
 #include "../lib/modding/ModIncompatibility.h"
 #include "../lib/rmg/CMapGenOptions.h"
@@ -646,11 +648,63 @@ void CServerHandler::startGameplay(VCMI_LIB_WRAP_NAMESPACE(CGameState) * gameSta
 	setState(EClientState::GAMEPLAY);
 }
 
+HighScoreParameter CServerHandler::prepareHighScores(PlayerColor player, bool victory)
+{
+	auto * gs = client->gameState();
+	auto * playerState = gs->getPlayerState(player);
+
+	HighScoreParameter param;
+	param.difficulty = gs->getStartInfo()->difficulty;
+	param.day = gs->getDate();
+	param.townAmount = gs->howManyTowns(player);
+	param.usedCheat = gs->getPlayerState(player)->cheated;
+	param.hasGrail = false;
+	for(const CGHeroInstance * h : playerState->heroes)
+		if(h->hasArt(ArtifactID::GRAIL))
+			param.hasGrail = true;
+	for(const CGTownInstance * t : playerState->towns)
+		if(t->builtBuildings.count(BuildingID::GRAIL))
+			param.hasGrail = true;
+	param.allDefeated = true;
+	for (PlayerColor otherPlayer(0); otherPlayer < PlayerColor::PLAYER_LIMIT; ++otherPlayer)
+	{
+		auto ps = gs->getPlayerState(otherPlayer, false);
+		if(ps && otherPlayer != player)
+			if(!ps->checkVanquished())
+				param.allDefeated = false;
+	}
+	param.scenarioName = gs->getMapHeader()->name.toString();
+	param.playerName = gs->getStartInfo()->playerInfos.find(player)->second.name;
+
+	return param;
+}
+
+void CServerHandler::showHighScoresAndEndGameplay(PlayerColor player, bool victory)
+{
+	HighScoreParameter param = prepareHighScores(player, victory);
+
+	if(victory && client->gameState()->getStartInfo()->campState)
+	{
+		startCampaignScenario(param, client->gameState()->getStartInfo()->campState);
+	}
+	else
+	{
+		HighScoreCalculation highScoreCalc;
+		highScoreCalc.parameters.push_back(param);
+		highScoreCalc.isCampaign = false;
+
+		endGameplay();
+		GH.defActionsDef = 63;
+		CMM->menu->switchToTab("main");
+		GH.windows().createAndPushWindow<CHighScoreInputScreen>(victory, highScoreCalc);
+	}
+}
+
 void CServerHandler::endGameplay()
 {
 	// Game is ending
 	// Tell the network thread to reach a stable state
-	CSH->sendClientDisconnecting();
+	sendClientDisconnecting();
 	logNetwork->info("Closed connection.");
 
 	client->endGame();
@@ -691,40 +745,37 @@ void CServerHandler::startCampaignScenario(HighScoreParameter param, std::shared
 	param.campaignName = cs->getNameTranslated();
 	highScoreCalc->parameters.push_back(param);
 
-	GH.dispatchMainThread([ourCampaign, this]()
-	{
-		CSH->endGameplay();
+	endGameplay();
 
-		auto & epilogue = ourCampaign->scenario(*ourCampaign->lastScenario()).epilog;
-		auto finisher = [=]()
+	auto & epilogue = ourCampaign->scenario(*ourCampaign->lastScenario()).epilog;
+	auto finisher = [=]()
+	{
+		if(ourCampaign->campaignSet != "" && ourCampaign->isCampaignFinished())
 		{
-			if(ourCampaign->campaignSet != "" && ourCampaign->isCampaignFinished())
-			{
-				Settings entry = persistentStorage.write["completedCampaigns"][ourCampaign->getFilename()];
-				entry->Bool() = true;
-			}
-
-			GH.windows().pushWindow(CMM);
-			GH.windows().pushWindow(CMM->menu);
+			Settings entry = persistentStorage.write["completedCampaigns"][ourCampaign->getFilename()];
+			entry->Bool() = true;
+		}
 
-			if(!ourCampaign->isCampaignFinished())
-				CMM->openCampaignLobby(ourCampaign);
-			else
-			{
-				CMM->openCampaignScreen(ourCampaign->campaignSet);
-				GH.windows().createAndPushWindow<CHighScoreInputScreen>(true, *highScoreCalc);
-			}
-		};
+		GH.windows().pushWindow(CMM);
+		GH.windows().pushWindow(CMM->menu);
 
-		if(epilogue.hasPrologEpilog)
-		{
-			GH.windows().createAndPushWindow<CPrologEpilogVideo>(epilogue, finisher);
-		}
+		if(!ourCampaign->isCampaignFinished())
+			CMM->openCampaignLobby(ourCampaign);
 		else
 		{
-			finisher();
+			CMM->openCampaignScreen(ourCampaign->campaignSet);
+			GH.windows().createAndPushWindow<CHighScoreInputScreen>(true, *highScoreCalc);
 		}
-	});
+	};
+
+	if(epilogue.hasPrologEpilog)
+	{
+		GH.windows().createAndPushWindow<CPrologEpilogVideo>(epilogue, finisher);
+	}
+	else
+	{
+		finisher();
+	}
 }
 
 void CServerHandler::showServerError(const std::string & txt) const
@@ -853,6 +904,14 @@ void CServerHandler::onPacketReceived(const std::shared_ptr<INetworkConnection>
 
 void CServerHandler::onDisconnected(const std::shared_ptr<INetworkConnection> & connection, const std::string & errorMessage)
 {
+	if (connection != networkConnection)
+	{
+		// ServerHandler already closed this connection on its own
+		// This is the final call from network thread that informs serverHandler that connection has died
+		// ignore it since serverHandler have already shut down this connection (and possibly started a new one)
+		return;
+	}
+
 	waitForServerShutdown();
 
 	if(getState() == EClientState::DISCONNECTING)

client/CServerHandler.h

@@ -128,6 +128,8 @@ class CServerHandler final : public IServerAPI, public LobbyInfo, public INetwor
 
 	bool isServerLocal() const;
 
+	HighScoreParameter prepareHighScores(PlayerColor player, bool victory);
+
 public:
 	/// High-level connection overlay that is capable of (de)serializing network data
 	std::shared_ptr<CConnection> logicConnection;
@@ -205,6 +207,7 @@ public:
 	void debugStartTest(std::string filename, bool save = false);
 
 	void startGameplay(VCMI_LIB_WRAP_NAMESPACE(CGameState) * gameState = nullptr);
+	void showHighScoresAndEndGameplay(PlayerColor player, bool victory);
 	void endGameplay();
 	void restartGameplay();
 	void startCampaignScenario(HighScoreParameter param, std::shared_ptr<CampaignState> cs = {});

client/NetPacksClient.cpp

@@ -15,6 +15,7 @@
 #include "CGameInfo.h"
 #include "windows/GUIClasses.h"
 #include "mapView/mapHandler.h"
+#include "adventureMap/AdventureMapInterface.h"
 #include "adventureMap/CInGameConsole.h"
 #include "battle/BattleInterface.h"
 #include "battle/BattleWindow.h"
@@ -408,6 +409,20 @@ void ApplyClientNetPackVisitor::visitPlayerEndsGame(PlayerEndsGame & pack)
 {
 	callAllInterfaces(cl, &IGameEventsReceiver::gameOver, pack.player, pack.victoryLossCheckResult);
 
+	bool lastHumanEndsGame = CSH->howManyPlayerInterfaces() == 1 && vstd::contains(cl.playerint, pack.player) && cl.getPlayerState(pack.player)->human && !settings["session"]["spectate"].Bool();
+
+	if (lastHumanEndsGame)
+	{
+		assert(adventureInt);
+		if(adventureInt)
+		{
+			GH.windows().popWindows(GH.windows().count());
+			adventureInt.reset();
+		}
+
+		CSH->showHighScoresAndEndGameplay(pack.player, pack.victoryLossCheckResult.victory());
+	}
+
 	// In auto testing pack.mode we always close client if red pack.player won or lose
 	if(!settings["session"]["testmap"].isNull() && pack.player == PlayerColor(0))
 	{

client/gui/WindowHandler.cpp

@@ -22,7 +22,9 @@
 
 void WindowHandler::popWindow(std::shared_ptr<IShowActivatable> top)
 {
-	assert(windowsStack.back() == top);
+	if (windowsStack.back() != top)
+		throw std::runtime_error("Attempt to pop non-top window from stack!");
+
 	top->deactivate();
 	disposed.push_back(top);
 	windowsStack.pop_back();
@@ -34,8 +36,11 @@ void WindowHandler::popWindow(std::shared_ptr<IShowActivatable> top)
 
 void WindowHandler::pushWindow(std::shared_ptr<IShowActivatable> newInt)
 {
-	assert(newInt);
-	assert(!vstd::contains(windowsStack, newInt)); // do not add same object twice
+	if (newInt == nullptr)
+		throw std::runtime_error("Attempt to push null window onto windows stack!");
+
+	if (vstd::contains(windowsStack, newInt))
+		throw std::runtime_error("Attempt to add already existing window to stack!");
 
 	//a new interface will be present, we'll need to use buffer surface (unless it's advmapint that will alter screenBuf on activate anyway)
 	screenBuf = screen2;

server/CVCMIServer.cpp

@@ -171,7 +171,11 @@ void CVCMIServer::onPacketReceived(const std::shared_ptr<INetworkConnection> & c
 
 void CVCMIServer::setState(EServerState value)
 {
-	assert(state != EServerState::SHUTDOWN); // do not attempt to restart dying server
+	if (value == EServerState::SHUTDOWN && state == EServerState::SHUTDOWN)
+		logGlobal->warn("Attempt to shutdown already shutdown server!");
+
+	// do not attempt to restart dying server
+	assert(state != EServerState::SHUTDOWN || state == value);
 	state = value;
 
 	if (state == EServerState::SHUTDOWN)