widelands-dev team mailing list archive

[SirVer <SirVer@xxxxxx>]

SirVer has proposed merging lp:~widelands-dev/widelands/different_replay_names into lp:widelands.

Commit message:
Give syncstreams and replays predictable names for each client instance.

Before, when one two clients joined the same network game from one computer, those clients would want to write to the same replay/syncstream file. In the best case this was a race condition, in the worst it could lead to crashes and corrupted replay/syncstrem files.

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1413577 in widelands: "Multiplayer client crashes - problem with file/directory: /binary/"
  https://bugs.launchpad.net/widelands/+bug/1413577

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/different_replay_names/+merge/285265
-- 
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/different_replay_names into lp:widelands.

=== modified file 'src/logic/game.cc'
--- src/logic/game.cc	2016-01-29 13:43:56 +0000
+++ src/logic/game.cc	2016-02-06 15:29:39 +0000
@@ -24,6 +24,7 @@
 #include <memory>
 #include <string>
 
+#include <boost/format.hpp>
 #ifndef _WIN32
 #include <SDL.h> // for a dirty hack.
 #include <unistd.h> // for usleep
@@ -73,10 +74,7 @@
 //#define SYNC_DEBUG
 
 Game::SyncWrapper::~SyncWrapper() {
-	if (m_dump) {
-		delete m_dump;
-		m_dump = nullptr;
-
+	if (m_dump != nullptr) {
 		if (!m_syncstreamsave)
 			g_fs->fs_unlink(m_dumpfname);
 	}
@@ -84,7 +82,7 @@
 
 void Game::SyncWrapper::start_dump(const std::string & fname) {
 	m_dumpfname = fname + ".wss";
-	m_dump = g_fs->open_stream_write(m_dumpfname);
+	m_dump.reset(g_fs->open_stream_write(m_dumpfname));
 }
 
 static const unsigned long long MINIMUM_DISK_SPACE = 256 * 1024 * 1024;
@@ -98,29 +96,21 @@
 	log("\n");
 #endif
 
-	if
-		(m_dump &&
-		 static_cast<int32_t>(m_counter - m_next_diskspacecheck) >= 0)
-	{
+	if (m_dump != nullptr && static_cast<int32_t>(m_counter - m_next_diskspacecheck) >= 0) {
 		m_next_diskspacecheck = m_counter + 16 * 1024 * 1024;
 
 		if (g_fs->disk_space() < MINIMUM_DISK_SPACE) {
 			log("Stop writing to syncstream file: disk is getting full.\n");
-			delete m_dump;
-			m_dump = nullptr;
+			m_dump.reset();
 		}
 	}
 
-	if (m_dump) {
+	if (m_dump != nullptr) {
 		try {
 			m_dump->data(sync_data, size);
 		} catch (const WException &) {
-			log
-				("Writing to syncstream file %s failed. Stop synctream dump.\n",
-				 m_dumpfname.c_str());
-
-			delete m_dump;
-			m_dump = nullptr;
+			log("Writing to syncstream file %s failed. Stop synctream dump.\n", m_dumpfname.c_str());
+			m_dump.reset();
 		}
 	}
 
@@ -137,7 +127,6 @@
 	m_writesyncstream     (false),
 	m_state               (gs_notrunning),
 	m_cmdqueue            (*this),
-	m_replaywriter        (nullptr),
 	/** TRANSLATORS: Win condition for this game has not been set. */
 	m_win_condition_displayname(_("Not set"))
 {
@@ -145,7 +134,6 @@
 
 Game::~Game()
 {
-	delete m_replaywriter;
 }
 
 
@@ -259,7 +247,7 @@
 
 	set_game_controller(new SinglePlayerGameController(*this, true, 1));
 	try {
-		bool const result = run(&loaderUI, NewSPScenario, script_to_run, false);
+		bool const result = run(&loaderUI, NewSPScenario, script_to_run, false, "single_player");
 		delete m_ctrl;
 		m_ctrl = nullptr;
 		return result;
@@ -427,7 +415,7 @@
 
 	set_game_controller(new SinglePlayerGameController(*this, true, player_nr));
 	try {
-		bool const result = run(&loaderUI, Loaded, script_to_run, false);
+		bool const result = run(&loaderUI, Loaded, script_to_run, false, "single_player");
 		delete m_ctrl;
 		m_ctrl = nullptr;
 		return result;
@@ -478,7 +466,7 @@
  */
 bool Game::run
 	(UI::ProgressWindow * loader_ui, StartGameType const start_game_type,
-	 const std::string& script_to_run, bool replay)
+	 const std::string& script_to_run, bool replay, const std::string& prefix_for_replays)
 {
 	m_replay = replay;
 	postload();
@@ -547,16 +535,13 @@
 
 	if (m_writereplay || m_writesyncstream) {
 		// Derive a replay filename from the current time
-		std::string fname(REPLAY_DIR);
-		fname += '/';
-		fname += timestring();
-		fname += REPLAY_SUFFIX;
-
+		const std::string fname = (boost::format("%s/%s_%s%s") % REPLAY_DIR % timestring() %
+		                           prefix_for_replays % REPLAY_SUFFIX).str();
 		if (m_writereplay) {
 			log("Starting replay writer\n");
 
 			assert(!m_replaywriter);
-			m_replaywriter = new ReplayWriter(*this, fname);
+			m_replaywriter.reset(new ReplayWriter(*this, fname));
 
 			log("Replay writer has started\n");
 		}

=== modified file 'src/logic/game.h'
--- src/logic/game.h	2015-11-11 09:53:54 +0000
+++ src/logic/game.h	2016-02-06 15:29:39 +0000
@@ -20,6 +20,8 @@
 #ifndef WL_LOGIC_GAME_H
 #define WL_LOGIC_GAME_H
 
+#include <memory>
+
 #include "base/md5.h"
 #include "io/streamwrite.h"
 #include "logic/cmd_queue.h"
@@ -108,7 +110,11 @@
 	void init_newgame (UI::ProgressWindow *, const GameSettings &);
 	void init_savegame(UI::ProgressWindow *, const GameSettings &);
 	enum StartGameType {NewSPScenario, NewNonScenario, Loaded, NewMPScenario};
-	bool run(UI::ProgressWindow * loader_ui, StartGameType, const std::string& script_to_run, bool replay);
+	bool run(UI::ProgressWindow* loader_ui,
+	         StartGameType,
+	         const std::string& script_to_run,
+	         bool replay,
+	         const std::string& prefix_for_replays);
 
 	// Returns the upcasted lua interface.
 	LuaGameInterface& lua() override;
@@ -127,7 +133,9 @@
 
 	void think() override;
 
-	ReplayWriter * get_replaywriter() {return m_replaywriter;}
+	ReplayWriter* get_replaywriter() {
+		return m_replaywriter.get();
+	}
 
 	/**
 	 * \return \c true if the game is completely loaded and running (or paused)
@@ -217,7 +225,6 @@
 			m_target        (target),
 			m_counter       (0),
 			m_next_diskspacecheck(0),
-			m_dump          (nullptr),
 			m_syncstreamsave(false)
 		{}
 
@@ -238,7 +245,7 @@
 		StreamWrite &   m_target;
 		uint32_t        m_counter;
 		uint32_t        m_next_diskspacecheck;
-		::StreamWrite * m_dump;
+		std::unique_ptr<::StreamWrite> m_dump;
 		std::string     m_dumpfname;
 		bool            m_syncstreamsave;
 	}                    m_syncwrapper;
@@ -263,8 +270,7 @@
 
 	SaveHandler          m_savehandler;
 
-	ReplayReader       * m_replayreader;
-	ReplayWriter       * m_replaywriter;
+	std::unique_ptr<ReplayWriter> m_replaywriter;
 
 	GeneralStatsVector m_general_stats;
 

=== modified file 'src/network/netclient.cc'
--- src/network/netclient.cc	2016-01-28 05:24:34 +0000
+++ src/network/netclient.cc	2016-02-06 15:29:39 +0000
@@ -219,13 +219,10 @@
 		d->lasttimestamp_realtime = SDL_GetTicks();
 
 		d->modal = game.get_ibase();
-		game.run
-			(loaderUI,
-			 d->settings.savegame ?
-			 Widelands::Game::Loaded
-			 : d->settings.scenario ?
-			 Widelands::Game::NewMPScenario : Widelands::Game::NewNonScenario,
-			 "", false);
+		game.run(loaderUI, d->settings.savegame ? Widelands::Game::Loaded : d->settings.scenario ?
+		                                          Widelands::Game::NewMPScenario :
+		                                          Widelands::Game::NewNonScenario,
+		         "", false, (boost::format("netclient_%d") % static_cast<int>(d->settings.usernum)).str());
 
 		// if this is an internet game, tell the metaserver that the game is done.
 		if (m_internet)

=== modified file 'src/network/nethost.cc'
--- src/network/nethost.cc	2016-02-01 08:21:18 +0000
+++ src/network/nethost.cc	2016-02-06 15:29:39 +0000
@@ -888,12 +888,11 @@
 						clients.push_back(d->settings.users.at(i).name);
 			DedicatedLog::get()->game_start(clients, game.map().get_name().c_str());
 		}
-		game.run
-			(loaderUI.get(),
-			 d->settings.savegame ? Widelands::Game::Loaded : d->settings.scenario ?
-			 Widelands::Game::NewMPScenario : Widelands::Game::NewNonScenario,
-			 "",
-			 false);
+		game.run(loaderUI.get(),
+		         d->settings.savegame ? Widelands::Game::Loaded : d->settings.scenario ?
+		                                Widelands::Game::NewMPScenario :
+		                                Widelands::Game::NewNonScenario,
+		         "", false, "nethost");
 
 		delete tips;
 

=== modified file 'src/wlapplication.cc'
--- src/wlapplication.cc	2016-02-04 15:37:19 +0000
+++ src/wlapplication.cc	2016-02-06 15:29:39 +0000
@@ -1297,7 +1297,7 @@
 
 			game.set_game_controller(ctrl.get());
 			game.init_newgame(&loaderUI, sp.settings());
-			game.run(&loaderUI, Widelands::Game::NewNonScenario, "", false);
+			game.run(&loaderUI, Widelands::Game::NewNonScenario, "", false, "single_player");
 		} catch (const std::exception & e) {
 			log("Fatal exception: %s\n", e.what());
 			emergency_save(game);
@@ -1412,7 +1412,7 @@
 
 		game.save_handler().set_allow_saving(false);
 
-		game.run(&loaderUI, Widelands::Game::Loaded, "", true);
+		game.run(&loaderUI, Widelands::Game::Loaded, "", true, "replay");
 	} catch (const std::exception & e) {
 		log("Fatal Exception: %s\n", e.what());
 		emergency_save(game);