widelands-dev team mailing list archive

[Arty <diego.scheide@xxxxxx>]

Arty has proposed merging lp:~widelands-dev/widelands/bug-1753230-working-with-tempfiles into lp:widelands.

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1753230 in widelands: "Scenarios: Some files get removed on autosave"
  https://bugs.launchpad.net/widelands/+bug/1753230

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1753230-working-with-tempfiles/+merge/357656
-- 
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1753230-working-with-tempfiles into lp:widelands.

=== modified file 'src/editor/editorinteractive.cc'
--- src/editor/editorinteractive.cc	2018-09-25 06:32:35 +0000
+++ src/editor/editorinteractive.cc	2018-10-22 18:50:51 +0000
@@ -187,6 +187,7 @@
 	}
 
 	ml->load_map_complete(egbase(), Widelands::MapLoader::LoadType::kEditor);
+	egbase().postload();
 	egbase().load_graphics(loader_ui);
 	map_changed(MapWas::kReplaced);
 }

=== modified file 'src/editor/ui_menus/main_menu_save_map.cc'
--- src/editor/ui_menus/main_menu_save_map.cc	2018-06-01 08:50:29 +0000
+++ src/editor/ui_menus/main_menu_save_map.cc	2018-10-22 18:50:51 +0000
@@ -272,16 +272,14 @@
 		if (mbox.run<UI::Panel::Returncodes>() == UI::Panel::Returncodes::kBack)
 			return false;
 	}
-
-	// save to a tmp file/dir first, rename later
-	// (important to keep script files in the script directory)
-	const std::string tmp_name = complete_filename + ".tmp";
-	if (g_fs->file_exists(tmp_name)) {
+	
+	//  Try deleting file (if it exists). If it fails, give a message and let the player choose a new name.
+	try {
+		g_fs->fs_unlink(complete_filename);
+	} catch (const std::exception& e) {
 		const std::string s =
-		   (boost::format(
-		       _("A file with the name ‘%s.tmp’ already exists. You have to remove it manually.")) %
-		    FileSystem::fs_filename(filename.c_str()))
-		      .str();
+		   (boost::format(_("File ‘%s.tmp’ could not be deleted.")) % FileSystem::fs_filename(filename.c_str())).str()
+		   + " " + _("Try saving under a different name!");
 		UI::WLMessageBox mbox(&eia(), _("Error Saving Map!"), s, UI::WLMessageBox::MBoxType::kOk);
 		mbox.run<UI::Panel::Returncodes>();
 		return false;
@@ -290,51 +288,35 @@
 	Widelands::EditorGameBase& egbase = eia().egbase();
 	Widelands::Map* map = egbase.mutable_map();
 
-	{  // fs scope
+	// Recompute seafaring tag
+	map->cleanup_port_spaces(egbase.world());
+	if (map->allows_seafaring()) {
+		map->add_tag("seafaring");
+	} else {
+		map->delete_tag("seafaring");
+	}
+
+	if (map->has_artifacts()) {
+		map->add_tag("artifacts");
+	} else {
+		map->delete_tag("artifacts");
+	}
+	
+	//  Try saving.
+	try {
 		std::unique_ptr<FileSystem> fs(
-		   g_fs->create_sub_file_system(tmp_name, binary ? FileSystem::ZIP : FileSystem::DIR));
-
-		// Recompute seafaring tag
-		map->cleanup_port_spaces(egbase.world());
-		if (map->allows_seafaring()) {
-			map->add_tag("seafaring");
-		} else {
-			map->delete_tag("seafaring");
-		}
-
-		if (map->has_artifacts()) {
-			map->add_tag("artifacts");
-		} else {
-			map->delete_tag("artifacts");
-		}
-
-		try {
-			Widelands::MapSaver* wms = new Widelands::MapSaver(*fs, egbase);
-			wms->save();
-			delete wms;
-			// Reset filesystem to avoid file locks on saves
-			fs.reset();
-			map->reset_filesystem();
-			eia().set_need_save(false);
-			g_fs->fs_unlink(complete_filename);
-			g_fs->fs_rename(tmp_name, complete_filename);
-			// Also change fs, as we assign it to the map below
-			fs.reset(g_fs->make_sub_file_system(complete_filename));
-			// Set the filesystem of the map to the current save file / directory
-			map->swap_filesystem(fs);
-			// DONT use fs as of here, its garbage now!
-
-		} catch (const std::exception& e) {
-			std::string s = _("Error Saving Map!\nSaved map file may be corrupt!\n\nReason "
-			                  "given:\n");
-			s += e.what();
-			UI::WLMessageBox mbox(&eia(), _("Error Saving Map!"), s, UI::WLMessageBox::MBoxType::kOk);
-			mbox.run<UI::Panel::Returncodes>();
-
-			// cleanup tmp file if it was created
-			g_fs->fs_unlink(tmp_name);
-		}
-	}  // end fs scope, dont use it
+		   g_fs->create_sub_file_system(complete_filename, binary ? FileSystem::ZIP : FileSystem::DIR));
+		Widelands::MapSaver* wms = new Widelands::MapSaver(*fs, egbase);
+		wms->save();
+		delete wms;
+		fs.reset();
+	} catch (const std::exception& e) {
+		std::string s = _("Error Saving Map!\nSaved map file may be corrupt!\n\nReason "
+		                  "given:\n");
+		s += e.what();
+		UI::WLMessageBox mbox(&eia(), _("Error Saving Map!"), s, UI::WLMessageBox::MBoxType::kOk);
+		mbox.run<UI::Panel::Returncodes>();
+	}
 
 	die();
 	return true;

=== modified file 'src/io/filesystem/zip_filesystem.cc'
--- src/io/filesystem/zip_filesystem.cc	2018-04-07 16:59:00 +0000
+++ src/io/filesystem/zip_filesystem.cc	2018-10-22 18:50:51 +0000
@@ -262,6 +262,9 @@
  * Create a sub filesystem out of this filesystem
  */
 FileSystem* ZipFilesystem::make_sub_file_system(const std::string& path) {
+	if (path == ".") {
+		return new ZipFilesystem(zip_file_, basedir_in_zip_file_);
+	}
 	if (!file_exists(path)) {
 		throw wexception(
 		   "ZipFilesystem::make_sub_file_system: The path '%s' does not exist in zip file '%s'.",

=== modified file 'src/logic/editor_game_base.cc'
--- src/logic/editor_game_base.cc	2018-09-28 05:41:33 +0000
+++ src/logic/editor_game_base.cc	2018-10-22 18:50:51 +0000
@@ -26,10 +26,12 @@
 #include "base/i18n.h"
 #include "base/macros.h"
 #include "base/scoped_timer.h"
+#include "base/time_string.h"
 #include "base/wexception.h"
 #include "economy/flag.h"
 #include "economy/road.h"
 #include "graphic/color.h"
+#include "logic/filesystem_constants.h"
 #include "logic/findimmovable.h"
 #include "logic/game.h"
 #include "logic/game_data_error.h"
@@ -48,6 +50,7 @@
 #include "logic/player.h"
 #include "logic/playersmanager.h"
 #include "logic/roadtype.h"
+#include "map_io/map_saver.h"
 #include "scripting/logic.h"
 #include "scripting/lua_table.h"
 #include "ui_basic/progresswindow.h"
@@ -67,12 +70,86 @@
    : gametime_(0),
      lua_(lua_interface),
      player_manager_(new PlayersManager(*this)),
-     ibase_(nullptr) {
+     ibase_(nullptr),
+     tmp_fs_(nullptr) {
 	if (!lua_)  // TODO(SirVer): this is sooo ugly, I can't say
 		lua_.reset(new LuaEditorInterface(this));
 }
 
 EditorGameBase::~EditorGameBase() {
+	delete_tempfile();
+}
+
+/**
+ * deletes the temporary file/dir
+ * also resets the map filesystem if it points to the temporary file
+ */
+void EditorGameBase::delete_tempfile() {
+	if (!tmp_fs_)
+		return;
+	std::string fs_filename = tmp_fs_->get_basename();
+	std::string mapfs_filename = map_.filesystem()->get_basename();
+	if (mapfs_filename == fs_filename)
+		map_.reset_filesystem();
+	tmp_fs_.reset();
+	try {
+		g_fs->fs_unlink(fs_filename);
+	} catch (const std::exception& e) {
+		// if file deletion fails then we have an abaondoned file lying around, but otherwise that's unproblematic
+		log("EditorGameBase::delete_tempfile: deleting temporary file/dir failed: %s\n", e.what());
+	}
+}
+
+/**
+ * creates a new file/dir, saves the map data, and reassigns the map filesystem
+ * does not delete the former temp file if one exists
+ * throws an exception if something goes wrong
+ */
+void EditorGameBase::create_tempfile_and_save_mapdata(FileSystem::Type const type) {
+  // should only be called when a map was already loaded
+	assert(map_.filesystem());
+
+	g_fs->ensure_directory_exists(kTempFileDir);
+
+	std::string filename = kTempFileDir + g_fs->file_separator() + timestring() + "_mapdata";
+	std::string complete_filename = filename + kTempFileExtension;
+
+	// if a file with that name already exists, then try a few name modifications
+	if (g_fs->file_exists(complete_filename))
+	{
+		int suffix;
+		for (suffix = 0; suffix <= 9; suffix++)
+		{
+			complete_filename = filename + "-" + std::to_string(suffix) + ".tmp";
+			if (!g_fs->file_exists(complete_filename))
+			  break;
+		}
+		if (suffix > 9) {
+		  throw wexception("EditorGameBase::create_tempfile_and_save_mapdata(): for all considered filenames a file already existed");
+		}
+	}
+
+	// create tmp_fs_
+	tmp_fs_.reset(g_fs->create_sub_file_system(complete_filename, type));
+
+	// save necessary map data (we actually save the whole map)
+	Widelands::MapSaver* wms = new Widelands::MapSaver(*tmp_fs_, *this);
+	wms->save();
+	delete wms;
+	
+	// swap map fs
+	std::unique_ptr<FileSystem> mapfs(tmp_fs_->make_sub_file_system("."));
+	map_.swap_filesystem(mapfs);
+	mapfs.reset();
+
+	// This is just a convenience hack:
+	// If tmp_fs_ is a zip filesystem then - because of the way zip filesystems are currently implemented -
+	// the file is still in zip mode right now, which means that the file isn't finalized yet, i.e.,
+	// not even a valid zip file until zip mode ends. To force ending the zip mode (thus finalizing the file)
+	// we simply perform a (otherwise useless) filesystem request.
+	// It's not strictly necessary, but this way we get a valid zip file immediately istead of
+	// at some unkown later point (when an unzip operation happens or a filesystem object destructs).
+	tmp_fs_->file_exists("binary");
 }
 
 void EditorGameBase::think() {
@@ -196,6 +273,16 @@
  * graphics are loaded.
  */
 void EditorGameBase::postload() {
+	if (map_.filesystem()) {
+		// save map data to temporary file and reassign map fs
+		try {
+			create_tempfile_and_save_mapdata(FileSystem::ZIP);
+		} catch (const WException& e) {
+			log("EditorGameBase::postload: saving map to temporary file failed: %s", e.what());
+			throw;
+		}
+	}
+
 	// Postload tribes
 	assert(tribes_);
 	tribes_->postload();
@@ -411,6 +498,8 @@
 	player_manager_->cleanup();
 
 	map_.cleanup();
+
+	delete_tempfile();
 }
 
 void EditorGameBase::set_road(const FCoords& f, uint8_t const direction, uint8_t const roadtype) {

=== modified file 'src/logic/editor_game_base.h'
--- src/logic/editor_game_base.h	2018-04-07 16:59:00 +0000
+++ src/logic/editor_game_base.h	2018-10-22 18:50:51 +0000
@@ -254,6 +254,15 @@
 	std::unique_ptr<Tribes> tribes_;
 	std::unique_ptr<InteractiveBase> ibase_;
 	Map map_;
+	
+	/// Even after a map is fully loaded, some static data (images, scripts)
+	/// will still be read from a filesystem whenever a map/game is saved.
+	/// To avoid potential filesystem conflicts when (pre)loading/saving/deleting
+	/// map/game files (and to avoid having to deal with this in many different places)
+	/// a temporary file (in a special dir) is created for such data.
+	std::unique_ptr<FileSystem> tmp_fs_;
+	void delete_tempfile();
+	void create_tempfile_and_save_mapdata(FileSystem::Type const type);
 
 	DISALLOW_COPY_AND_ASSIGN(EditorGameBase);
 };

=== modified file 'src/logic/filesystem_constants.h'
--- src/logic/filesystem_constants.h	2018-04-22 16:01:32 +0000
+++ src/logic/filesystem_constants.h	2018-10-22 18:50:51 +0000
@@ -38,6 +38,13 @@
 const std::string kS2MapExtension1 = ".swd";
 const std::string kS2MapExtension2 = ".wld";
 
+/// Filesystem names for temp files holding static data that needs to be accessible via filesystem
+/// Kept in a separate dir to avoid filesystem conflicts
+const std::string kTempFileDir = "in_progress";
+const std::string kTempFileExtension = ".tmp";
+// We delete (accidentally remaining) temp files older than a week
+constexpr double kTempFilesKeepAroundTime = 7 * 24 * 60 * 60;
+
 /// Filesystem names and timeouts for replays
 const std::string kReplayDir = "replays";
 const std::string kReplayExtension = ".wrpl";

=== modified file 'src/wlapplication.cc'
--- src/wlapplication.cc	2018-10-09 17:09:34 +0000
+++ src/wlapplication.cc	2018-10-22 18:50:51 +0000
@@ -339,6 +339,7 @@
 	changedir_on_mac();
 	cleanup_replays();
 	cleanup_ai_files();
+	cleanup_temp_files();
 
 	Section& config = g_options.pull_section("global");
 
@@ -1498,6 +1499,21 @@
 	}
 }
 
+/**
+ * Delete old temp files that might still lurk around (game crashes etc.)
+ */
+void WLApplication::cleanup_temp_files() {
+	for (const std::string& filename :
+	     filter(g_fs->list_directory(kTempFileDir), [](const std::string& fn) {
+		     return boost::ends_with(fn, kTempFileExtension);
+		  })) {
+		if (is_autogenerated_and_expired(filename, kTempFilesKeepAroundTime)) {
+			log("Deleting old temp file: %s\n", filename.c_str());
+			g_fs->fs_unlink(filename);
+		}
+	}
+}
+
 bool WLApplication::redirect_output(std::string path) {
 	if (path.empty()) {
 #ifdef _WIN32

=== modified file 'src/wlapplication.h'
--- src/wlapplication.h	2018-04-07 16:59:00 +0000
+++ src/wlapplication.h	2018-10-22 18:50:51 +0000
@@ -216,6 +216,8 @@
 
 	void cleanup_ai_files();
 
+	void cleanup_temp_files();
+
 	bool redirect_output(std::string path = "");
 
 	// Handle the given pressed key. Returns true when key was