Merge lp:~widelands-dev/widelands/bug-1753230-working-with-tempfiles into lp:widelands

[Arty]

Fix bugs with missing files/folders in savegames

- Whenever a map is fully loaded (editor/game/replay) the map is
  immediately saved to a temp file and the map's filesystem is
  reassigned to that temp file in a special directory.
  So, when map/save files are handled anywhere, there won't be any
  filesystem conflicts with or even accidentally deletion of the map
  filesystem (which would lead to corrupt save files later).

- Temp files are automatically deleted when not needed any more.

- Upon application start, any found temp files that are older than a week
  are considered accidental leftovers (e.g. from crashes) and deleted.

[GunChleoc (gunchleoc)]

Code LGTM - just some minor nits.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 4161. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/444762108.
Appveyor build 3959. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1753230_working_with_tempfiles-3959.

[GunChleoc (gunchleoc)]

I have tested this by allowing only 1 autosave file and sitting on wl_autosave_00.wgf with 7zip on Windows. Since wl_autosave_01.wgf - wl_autosave_05.wgf already existed, the autosaves that I got were wl_autosave_06.wgf - wl_autosave_09.wgf

I think this branch is good to go in after the other nits have been fixed, because it already improves the situation greatly, but maybe we could have some form of "is_writable" test rather than "file_exists"?

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 4183. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/449653297.
Appveyor build 3981. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1753230_working_with_tempfiles-3981.

[Arty (artydent)]

1. Regarding the autosave stuff:

This wasn't anything this branch tried to solve. It only solved the the conflicts that arose from trying to modify files where the map fs pointed to, resulting in either some crash or error (if the file was currently locked) or resulting in corrupt savefiles.

Those other file conflicts (like trying to modify a savefile that is locked because you have it open with 7zip, or having a file write-protected or some such) are solved with the latest branch: https://code.launchpad.net/~widelands-dev/widelands/robust-saving

As for trying to specifically test "is_writable" instead of "file_exists" (or other potentially problematic file states) beforehand, I don't think it's worth it. For one thing, we might have to deal with the specifics of the various access control models on the different platforms. (Just looking at this stuff makes me nauseaous.) There is also the more general problem with any filesystem stuff that you can never be sure that the file state you just checked is still the same when you try your file operation next. Sure, it's very likely, but there can always be race conditions, and some other process might just have snatched the file away between your state check and your attempt to operate on a file.
So aside from a few minor exist checks I favour a simple, practical approach: just try the file operation, but catch and handle any errors. The error codes give a hint about what the actual problem was, but usually that doesn't matter much anyway: either it worked and we continue or it didn't and we abort.
Anyway, the latest branch https://code.launchpad.net/~widelands-dev/widelands/robust-saving does exactly that and handles file errors in all the map/game saving routines and cleanup routines. I still need to check in what other places we don't catch file errors but should. (Definitely when deleting files in the load/save menu, maybe synchstream creation, probably some other places.) That's going to be part of future branches though.

2. Those travis build errors

I hadn't actually seen this before. (I'm still a noob on Launchpad and don't know my way around very well, but I am trying to catch up. If there are any important features I might not but should know about, feel free to point something out.)

Anyway, those errors seem like minor nitpicks. I'll fix them.
Btw, are those builds made automatically here for any new branches? Or do I have to initiate them manually? (I haven't checked out the online building possibilities yet. Still on my todo list.)

[Arty (artydent)]

Btw, this autosave-thing you observed was a (I think even relatively recently introduced and somewhat dirty) error handling attempt by someone. When the wl_autosave_00.wgf couldn't be renamed (for backup purposes) then there was some "let's just try other filenames" approach, and the game would just find the next autosave filename (up to 09) where there didn't exist any file. At 09 it would just stop and not check any further, but use this as the target name, so from this point on wl_autosave_09.wgf would just have been overwritten with every autosave. (Which worked in your case, because this one wasn't file-locked.)

My latest branch is stricter in this regard. If some relevant renaming or deletion fails, then the autosaving is simply aborted, and there is a little message. I don't think the game needs to handle this in some special way. The player gets a message and can try to fix a problem (if there's a file lock for example) or has to save manually.

[GunChleoc (gunchleoc)]

The automatic builds are triggered by every push to a branch that has an active merge request.

For our testing environment, including our codecheck, see:

https://wl.widelands.org/wiki/RegressionTests/

If you scroll up to my first comment, you'll see a green "Show diff comments" link. Click on it and you'll see my comments in the diff. For longer diffs, I use the browser's search function and search for the reviewer's username to skip through the comments.

I agree with your reasoning about an "is writable" check, let's keep this branch as is. I'd like my diff comments addressed though.

[GunChleoc (gunchleoc)]

You have introduces a "rnrnrn " string in your last commit in a license header ;)

[Arty (artydent)]

No, I didn't. :-)

The string is already in the last trunk. I just noticed it and removed it together with the other small fix I commited. (It just didn't feel important enough to mention it in the log.)

[GunChleoc (gunchleoc)]

You're right, I read the diff the wrong way around. Thanks for fixing this too :)

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 4188. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/450208008.
Appveyor build 3985. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1753230_working_with_tempfiles-3985.

[GunChleoc (gunchleoc)]

I have now fixed the nits myself, so that we can start reviewing your other branches

@bunnybot merge

review: Approve

[Arty (artydent)]

Huh? I had already fixed them. Did I miss something?

[Arty (artydent)]

Oh, I see you had some other nits that you fixed. Fair enough. I should have checked more thoroughly. Or did you mention some specifics before somewhere and I had missed them?

As for the naming of the temp dir, I also had it named "temp" first but felt that might encourage players to just delete it (possibly while the game is up, thus messing up their next save). I guess I was overreacting because a friend of mine notoriously does this kind of thing, always complaining that stupid programmers never clean up their temp stuff. But honestly, "temp" is perfectly fine.

[Arty (artydent)]

Oh, now I see where your original comments are. Don't know why I didn't notice them before. Sorry about that.

[GunChleoc (gunchleoc)]

Don't worry about it - I know you're still finding your way around Launchpad and I'm happy to help.

If somebody decides to mess with the temp while playing, on their own head be it. At least everybody else will know it's temp files if they want to manually clean it out at a sensible point in time ;)

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 4203. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/452768811.
Appveyor build 3999. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1753230_working_with_tempfiles-3999.

[bunnybot (widelandsofficial)]

Refusing to merge, since Travis is not green. Use @bunnybot merge force for merging anyways.

Travis build 4203. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/452768811.

[GunChleoc (gunchleoc)]

Travis error is a timeout

@bunnybot merge force

=== modified file 'src/editor/editorinteractive.cc'
--- src/editor/editorinteractive.cc	2018-09-25 06:32:35 +0000
+++ src/editor/editorinteractive.cc	2018-11-09 08:01:52 +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-11-09 08:01:52 +0000
@@ -273,15 +273,14 @@
 			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) {
+        log("Unable to delete old map file %s while saving map: %s\n", complete_filename.c_str(), e.what());
 		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 ‘%’ 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 +289,34 @@
 	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));
+		std::unique_ptr<Widelands::MapSaver> wms(new Widelands::MapSaver(*fs, egbase));
+		wms->save();
+		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-11-09 08:01:52 +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-11-09 08:01:52 +0000
@@ -8,7 +8,7 @@
  *
  * This program is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
-rnrnrn * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
@@ -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,87 @@
    : 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 abandoned 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) + kTempFileExtension;
+			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)
+    std::unique_ptr<Widelands::MapSaver> wms(new Widelands::MapSaver(*tmp_fs_, *this));
+	wms->save();
+
+	// 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 +274,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 +499,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-11-09 08:01:52 +0000
@@ -255,6 +255,15 @@
 	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 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-11-09 08:01:52 +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 = "temp";
+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-11-09 08:01:52 +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-11-09 08:01:52 +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