From 80acd7e77ccf91434ff239291683bfb596eeeccc Mon Sep 17 00:00:00 2001 From: Ivan Savenko Date: Sun, 7 Apr 2024 14:19:57 +0300 Subject: [PATCH 1/3] 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 | 69 --------------------- client/CServerHandler.cpp | 117 ++++++++++++++++++++++++++--------- client/CServerHandler.h | 3 + client/NetPacksClient.cpp | 15 +++++ client/gui/WindowHandler.cpp | 11 +++- server/CVCMIServer.cpp | 6 +- 6 files changed, 119 insertions(+), 102 deletions(-) diff --git a/client/CPlayerInterface.cpp b/client/CPlayerInterface.cpp index 4c0a2d4b4..66b9ac462 100644 --- a/client/CPlayerInterface.cpp +++ b/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(won, highScoreCalc); - } - ); - } -} - void CPlayerInterface::askToAssembleArtifact(const ArtifactLocation &al) { if(auto hero = cb->getHero(al.artHolder)) diff --git a/client/CServerHandler.cpp b/client/CServerHandler.cpp index 1ed2aa717..4fc4e9bee 100644 --- a/client/CServerHandler.cpp +++ b/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(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]() + endGameplay(); + + auto & epilogue = ourCampaign->scenario(*ourCampaign->lastScenario()).epilog; + auto finisher = [=]() { - CSH->endGameplay(); - - 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); - - if(!ourCampaign->isCampaignFinished()) - CMM->openCampaignLobby(ourCampaign); - else - { - CMM->openCampaignScreen(ourCampaign->campaignSet); - GH.windows().createAndPushWindow(true, *highScoreCalc); - } - }; - - if(epilogue.hasPrologEpilog) - { - GH.windows().createAndPushWindow(epilogue, finisher); + Settings entry = persistentStorage.write["completedCampaigns"][ourCampaign->getFilename()]; + entry->Bool() = true; } + + GH.windows().pushWindow(CMM); + GH.windows().pushWindow(CMM->menu); + + if(!ourCampaign->isCampaignFinished()) + CMM->openCampaignLobby(ourCampaign); else { - finisher(); + CMM->openCampaignScreen(ourCampaign->campaignSet); + GH.windows().createAndPushWindow(true, *highScoreCalc); } - }); + }; + + if(epilogue.hasPrologEpilog) + { + GH.windows().createAndPushWindow(epilogue, finisher); + } + else + { + finisher(); + } } void CServerHandler::showServerError(const std::string & txt) const @@ -853,6 +904,14 @@ void CServerHandler::onPacketReceived(const std::shared_ptr void CServerHandler::onDisconnected(const std::shared_ptr & 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) diff --git a/client/CServerHandler.h b/client/CServerHandler.h index 265d153d2..95ba1f026 100644 --- a/client/CServerHandler.h +++ b/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 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 cs = {}); diff --git a/client/NetPacksClient.cpp b/client/NetPacksClient.cpp index 6ebcab9a9..34f5cd8ca 100644 --- a/client/NetPacksClient.cpp +++ b/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)) { diff --git a/client/gui/WindowHandler.cpp b/client/gui/WindowHandler.cpp index 522155d19..ba8066089 100644 --- a/client/gui/WindowHandler.cpp +++ b/client/gui/WindowHandler.cpp @@ -22,7 +22,9 @@ void WindowHandler::popWindow(std::shared_ptr 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 top) void WindowHandler::pushWindow(std::shared_ptr 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; diff --git a/server/CVCMIServer.cpp b/server/CVCMIServer.cpp index c57a588d7..f546f477f 100644 --- a/server/CVCMIServer.cpp +++ b/server/CVCMIServer.cpp @@ -171,7 +171,11 @@ void CVCMIServer::onPacketReceived(const std::shared_ptr & 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) From 0a296add0cd971185fedcbd9d02796152a96b627 Mon Sep 17 00:00:00 2001 From: Ivan Savenko Date: Sun, 7 Apr 2024 21:21:48 +0300 Subject: [PATCH 2/3] apply SonarCloud suggestions --- client/CServerHandler.cpp | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/client/CServerHandler.cpp b/client/CServerHandler.cpp index 4fc4e9bee..73ad5d43e 100644 --- a/client/CServerHandler.cpp +++ b/client/CServerHandler.cpp @@ -650,8 +650,8 @@ void CServerHandler::startGameplay(VCMI_LIB_WRAP_NAMESPACE(CGameState) * gameSta HighScoreParameter CServerHandler::prepareHighScores(PlayerColor player, bool victory) { - auto * gs = client->gameState(); - auto * playerState = gs->getPlayerState(player); + const auto * gs = client->gameState(); + const auto * playerState = gs->getPlayerState(player); HighScoreParameter param; param.difficulty = gs->getStartInfo()->difficulty; @@ -669,9 +669,8 @@ HighScoreParameter CServerHandler::prepareHighScores(PlayerColor player, bool vi 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; + if(ps && otherPlayer != player && !ps->checkVanquished()) + param.allDefeated = false; } param.scenarioName = gs->getMapHeader()->name.toString(); param.playerName = gs->getStartInfo()->playerInfos.find(player)->second.name; @@ -689,14 +688,14 @@ void CServerHandler::showHighScoresAndEndGameplay(PlayerColor player, bool victo } else { - HighScoreCalculation highScoreCalc; - highScoreCalc.parameters.push_back(param); - highScoreCalc.isCampaign = false; + HighScoreCalculation scenarioHighScores; + scenarioHighScores.parameters.push_back(param); + scenarioHighScores.isCampaign = false; endGameplay(); GH.defActionsDef = 63; CMM->menu->switchToTab("main"); - GH.windows().createAndPushWindow(victory, highScoreCalc); + GH.windows().createAndPushWindow(victory, scenarioHighScores); } } @@ -748,7 +747,7 @@ void CServerHandler::startCampaignScenario(HighScoreParameter param, std::shared endGameplay(); auto & epilogue = ourCampaign->scenario(*ourCampaign->lastScenario()).epilog; - auto finisher = [=]() + auto finisher = [this, ourCampaign]() { if(ourCampaign->campaignSet != "" && ourCampaign->isCampaignFinished()) { From e89e5d21275c331f75e6a27da838a1b5bf35c9a3 Mon Sep 17 00:00:00 2001 From: Ivan Savenko Date: Sun, 7 Apr 2024 21:22:33 +0300 Subject: [PATCH 3/3] Rename for clarity --- client/CServerHandler.cpp | 19 ++++++++++++------- client/CServerHandler.h | 3 ++- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/client/CServerHandler.cpp b/client/CServerHandler.cpp index 73ad5d43e..685a90187 100644 --- a/client/CServerHandler.cpp +++ b/client/CServerHandler.cpp @@ -146,6 +146,11 @@ CServerHandler::CServerHandler() registerTypesLobbyPacks(*applier); } +void CServerHandler::setHighScoreCalc(const std::shared_ptr &newHighScoreCalc) +{ + campaignScoreCalculator = newHighScoreCalc; +} + void CServerHandler::threadRunNetwork() { logGlobal->info("Starting network thread"); @@ -627,7 +632,7 @@ void CServerHandler::startGameplay(VCMI_LIB_WRAP_NAMESPACE(CGameState) * gameSta if(CMM) CMM->disable(); - highScoreCalc = nullptr; + campaignScoreCalculator = nullptr; switch(si->mode) { @@ -735,14 +740,14 @@ void CServerHandler::startCampaignScenario(HighScoreParameter param, std::shared if (!cs) ourCampaign = si->campState; - if(highScoreCalc == nullptr) + if(campaignScoreCalculator == nullptr) { - highScoreCalc = std::make_shared(); - highScoreCalc->isCampaign = true; - highScoreCalc->parameters.clear(); + campaignScoreCalculator = std::make_shared(); + campaignScoreCalculator->isCampaign = true; + campaignScoreCalculator->parameters.clear(); } param.campaignName = cs->getNameTranslated(); - highScoreCalc->parameters.push_back(param); + campaignScoreCalculator->parameters.push_back(param); endGameplay(); @@ -763,7 +768,7 @@ void CServerHandler::startCampaignScenario(HighScoreParameter param, std::shared else { CMM->openCampaignScreen(ourCampaign->campaignSet); - GH.windows().createAndPushWindow(true, *highScoreCalc); + GH.windows().createAndPushWindow(true, *campaignScoreCalculator); } }; diff --git a/client/CServerHandler.h b/client/CServerHandler.h index 95ba1f026..cbf87eaee 100644 --- a/client/CServerHandler.h +++ b/client/CServerHandler.h @@ -106,7 +106,7 @@ class CServerHandler final : public IServerAPI, public LobbyInfo, public INetwor std::unique_ptr serverRunner; std::shared_ptr mapToStart; std::vector localPlayerNames; - std::shared_ptr highScoreCalc; + std::shared_ptr campaignScoreCalculator; boost::thread threadNetwork; @@ -219,6 +219,7 @@ public: void visitForLobby(CPackForLobby & lobbyPack); void visitForClient(CPackForClient & clientPack); + void setHighScoreCalc(const std::shared_ptr &newHighScoreCalc); }; extern CServerHandler * CSH;