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

Proposed by Arty
Status: Merged
Merged at revision: 8917
Proposed branch: lp:~widelands-dev/widelands/bug-1753230-working-with-tempfiles
Merge into: lp:widelands
Diff against target: 365 lines (+164/-54)
8 files modified
src/editor/editorinteractive.cc (+1/-0)
src/editor/ui_menus/main_menu_save_map.cc (+34/-52)
src/io/filesystem/zip_filesystem.cc (+3/-0)
src/logic/editor_game_base.cc (+92/-2)
src/logic/editor_game_base.h (+9/-0)
src/logic/filesystem_constants.h (+7/-0)
src/wlapplication.cc (+16/-0)
src/wlapplication.h (+2/-0)
To merge this branch: bzr merge lp:~widelands-dev/widelands/bug-1753230-working-with-tempfiles
Reviewer Review Type Date Requested Status
GunChleoc Approve
Review via email: mp+357656@code.launchpad.net

Commit message

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.

To post a comment you must log in.
Revision history for this message
GunChleoc (gunchleoc) wrote :

Code LGTM - just some minor nits.

Revision history for this message
bunnybot (widelandsofficial) wrote :

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.

Revision history for this message
GunChleoc (gunchleoc) wrote :

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"?

8897. By artydent

Merged trunk.

Revision history for this message
bunnybot (widelandsofficial) wrote :

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.

Revision history for this message
Arty (artydent) wrote :

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.)

Revision history for this message
Arty (artydent) wrote :

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.

8898. By artydent

make codecheck happy

Revision history for this message
GunChleoc (gunchleoc) wrote :

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.

8899. By artydent

=small fix: changed a literal to a filesystem-constant

Revision history for this message
GunChleoc (gunchleoc) wrote :

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

Revision history for this message
Arty (artydent) wrote :

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.)

Revision history for this message
GunChleoc (gunchleoc) wrote :

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

Revision history for this message
bunnybot (widelandsofficial) wrote :

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.

8900. By GunChleoc

Fixed the nits

8901. By GunChleoc

Merged trunk.

Revision history for this message
GunChleoc (gunchleoc) wrote :

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

@bunnybot merge

review: Approve
Revision history for this message
Arty (artydent) wrote :

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

Revision history for this message
Arty (artydent) wrote :

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.

Revision history for this message
Arty (artydent) wrote :

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

Revision history for this message
GunChleoc (gunchleoc) wrote :

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 ;)

Revision history for this message
bunnybot (widelandsofficial) wrote :

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.

Revision history for this message
bunnybot (widelandsofficial) wrote :

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.

Revision history for this message
GunChleoc (gunchleoc) wrote :

Travis error is a timeout

@bunnybot merge force

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/editor/editorinteractive.cc'
2--- src/editor/editorinteractive.cc 2018-09-25 06:32:35 +0000
3+++ src/editor/editorinteractive.cc 2018-11-09 08:01:52 +0000
4@@ -187,6 +187,7 @@
5 }
6
7 ml->load_map_complete(egbase(), Widelands::MapLoader::LoadType::kEditor);
8+ egbase().postload();
9 egbase().load_graphics(loader_ui);
10 map_changed(MapWas::kReplaced);
11 }
12
13=== modified file 'src/editor/ui_menus/main_menu_save_map.cc'
14--- src/editor/ui_menus/main_menu_save_map.cc 2018-06-01 08:50:29 +0000
15+++ src/editor/ui_menus/main_menu_save_map.cc 2018-11-09 08:01:52 +0000
16@@ -273,15 +273,14 @@
17 return false;
18 }
19
20- // save to a tmp file/dir first, rename later
21- // (important to keep script files in the script directory)
22- const std::string tmp_name = complete_filename + ".tmp";
23- if (g_fs->file_exists(tmp_name)) {
24+ // Try deleting file (if it exists). If it fails, give a message and let the player choose a new name.
25+ try {
26+ g_fs->fs_unlink(complete_filename);
27+ } catch (const std::exception& e) {
28+ log("Unable to delete old map file %s while saving map: %s\n", complete_filename.c_str(), e.what());
29 const std::string s =
30- (boost::format(
31- _("A file with the name ‘%s.tmp’ already exists. You have to remove it manually.")) %
32- FileSystem::fs_filename(filename.c_str()))
33- .str();
34+ (boost::format(_("File ‘%’ could not be deleted.")) % FileSystem::fs_filename(filename.c_str())).str()
35+ + " " + _("Try saving under a different name!");
36 UI::WLMessageBox mbox(&eia(), _("Error Saving Map!"), s, UI::WLMessageBox::MBoxType::kOk);
37 mbox.run<UI::Panel::Returncodes>();
38 return false;
39@@ -290,51 +289,34 @@
40 Widelands::EditorGameBase& egbase = eia().egbase();
41 Widelands::Map* map = egbase.mutable_map();
42
43- { // fs scope
44+ // Recompute seafaring tag
45+ map->cleanup_port_spaces(egbase.world());
46+ if (map->allows_seafaring()) {
47+ map->add_tag("seafaring");
48+ } else {
49+ map->delete_tag("seafaring");
50+ }
51+
52+ if (map->has_artifacts()) {
53+ map->add_tag("artifacts");
54+ } else {
55+ map->delete_tag("artifacts");
56+ }
57+
58+ // Try saving.
59+ try {
60 std::unique_ptr<FileSystem> fs(
61- g_fs->create_sub_file_system(tmp_name, binary ? FileSystem::ZIP : FileSystem::DIR));
62-
63- // Recompute seafaring tag
64- map->cleanup_port_spaces(egbase.world());
65- if (map->allows_seafaring()) {
66- map->add_tag("seafaring");
67- } else {
68- map->delete_tag("seafaring");
69- }
70-
71- if (map->has_artifacts()) {
72- map->add_tag("artifacts");
73- } else {
74- map->delete_tag("artifacts");
75- }
76-
77- try {
78- Widelands::MapSaver* wms = new Widelands::MapSaver(*fs, egbase);
79- wms->save();
80- delete wms;
81- // Reset filesystem to avoid file locks on saves
82- fs.reset();
83- map->reset_filesystem();
84- eia().set_need_save(false);
85- g_fs->fs_unlink(complete_filename);
86- g_fs->fs_rename(tmp_name, complete_filename);
87- // Also change fs, as we assign it to the map below
88- fs.reset(g_fs->make_sub_file_system(complete_filename));
89- // Set the filesystem of the map to the current save file / directory
90- map->swap_filesystem(fs);
91- // DONT use fs as of here, its garbage now!
92-
93- } catch (const std::exception& e) {
94- std::string s = _("Error Saving Map!\nSaved map file may be corrupt!\n\nReason "
95- "given:\n");
96- s += e.what();
97- UI::WLMessageBox mbox(&eia(), _("Error Saving Map!"), s, UI::WLMessageBox::MBoxType::kOk);
98- mbox.run<UI::Panel::Returncodes>();
99-
100- // cleanup tmp file if it was created
101- g_fs->fs_unlink(tmp_name);
102- }
103- } // end fs scope, dont use it
104+ g_fs->create_sub_file_system(complete_filename, binary ? FileSystem::ZIP : FileSystem::DIR));
105+ std::unique_ptr<Widelands::MapSaver> wms(new Widelands::MapSaver(*fs, egbase));
106+ wms->save();
107+ fs.reset();
108+ } catch (const std::exception& e) {
109+ std::string s = _("Error Saving Map!\nSaved map file may be corrupt!\n\nReason "
110+ "given:\n");
111+ s += e.what();
112+ UI::WLMessageBox mbox(&eia(), _("Error Saving Map!"), s, UI::WLMessageBox::MBoxType::kOk);
113+ mbox.run<UI::Panel::Returncodes>();
114+ }
115
116 die();
117 return true;
118
119=== modified file 'src/io/filesystem/zip_filesystem.cc'
120--- src/io/filesystem/zip_filesystem.cc 2018-04-07 16:59:00 +0000
121+++ src/io/filesystem/zip_filesystem.cc 2018-11-09 08:01:52 +0000
122@@ -262,6 +262,9 @@
123 * Create a sub filesystem out of this filesystem
124 */
125 FileSystem* ZipFilesystem::make_sub_file_system(const std::string& path) {
126+ if (path == ".") {
127+ return new ZipFilesystem(zip_file_, basedir_in_zip_file_);
128+ }
129 if (!file_exists(path)) {
130 throw wexception(
131 "ZipFilesystem::make_sub_file_system: The path '%s' does not exist in zip file '%s'.",
132
133=== modified file 'src/logic/editor_game_base.cc'
134--- src/logic/editor_game_base.cc 2018-09-28 05:41:33 +0000
135+++ src/logic/editor_game_base.cc 2018-11-09 08:01:52 +0000
136@@ -8,7 +8,7 @@
137 *
138 * This program is distributed in the hope that it will be useful,
139 * but WITHOUT ANY WARRANTY; without even the implied warranty of
140-rnrnrn * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
141+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
142 * GNU General Public License for more details.
143 *
144 * You should have received a copy of the GNU General Public License
145@@ -26,10 +26,12 @@
146 #include "base/i18n.h"
147 #include "base/macros.h"
148 #include "base/scoped_timer.h"
149+#include "base/time_string.h"
150 #include "base/wexception.h"
151 #include "economy/flag.h"
152 #include "economy/road.h"
153 #include "graphic/color.h"
154+#include "logic/filesystem_constants.h"
155 #include "logic/findimmovable.h"
156 #include "logic/game.h"
157 #include "logic/game_data_error.h"
158@@ -48,6 +50,7 @@
159 #include "logic/player.h"
160 #include "logic/playersmanager.h"
161 #include "logic/roadtype.h"
162+#include "map_io/map_saver.h"
163 #include "scripting/logic.h"
164 #include "scripting/lua_table.h"
165 #include "ui_basic/progresswindow.h"
166@@ -67,12 +70,87 @@
167 : gametime_(0),
168 lua_(lua_interface),
169 player_manager_(new PlayersManager(*this)),
170- ibase_(nullptr) {
171+ ibase_(nullptr),
172+ tmp_fs_(nullptr) {
173 if (!lua_) // TODO(SirVer): this is sooo ugly, I can't say
174 lua_.reset(new LuaEditorInterface(this));
175 }
176
177 EditorGameBase::~EditorGameBase() {
178+ delete_tempfile();
179+}
180+
181+/**
182+ * deletes the temporary file/dir
183+ * also resets the map filesystem if it points to the temporary file
184+ */
185+void EditorGameBase::delete_tempfile() {
186+ if (!tmp_fs_) {
187+ return;
188+ }
189+
190+ std::string fs_filename = tmp_fs_->get_basename();
191+ std::string mapfs_filename = map_.filesystem()->get_basename();
192+ if (mapfs_filename == fs_filename)
193+ map_.reset_filesystem();
194+ tmp_fs_.reset();
195+ try {
196+ g_fs->fs_unlink(fs_filename);
197+ } catch (const std::exception& e) {
198+ // if file deletion fails then we have an abandoned file lying around, but otherwise that's unproblematic
199+ log("EditorGameBase::delete_tempfile: deleting temporary file/dir failed: %s\n", e.what());
200+ }
201+}
202+
203+/**
204+ * creates a new file/dir, saves the map data, and reassigns the map filesystem
205+ * does not delete the former temp file if one exists
206+ * throws an exception if something goes wrong
207+ */
208+void EditorGameBase::create_tempfile_and_save_mapdata(FileSystem::Type const type) {
209+ // should only be called when a map was already loaded
210+ assert(map_.filesystem());
211+
212+ g_fs->ensure_directory_exists(kTempFileDir);
213+
214+ std::string filename = kTempFileDir + g_fs->file_separator() + timestring() + "_mapdata";
215+ std::string complete_filename = filename + kTempFileExtension;
216+
217+ // if a file with that name already exists, then try a few name modifications
218+ if (g_fs->file_exists(complete_filename))
219+ {
220+ int suffix;
221+ for (suffix = 0; suffix <= 9; suffix++)
222+ {
223+ complete_filename = filename + "-" + std::to_string(suffix) + kTempFileExtension;
224+ if (!g_fs->file_exists(complete_filename))
225+ break;
226+ }
227+ if (suffix > 9) {
228+ throw wexception("EditorGameBase::create_tempfile_and_save_mapdata(): for all considered filenames a file already existed");
229+ }
230+ }
231+
232+ // create tmp_fs_
233+ tmp_fs_.reset(g_fs->create_sub_file_system(complete_filename, type));
234+
235+ // save necessary map data (we actually save the whole map)
236+ std::unique_ptr<Widelands::MapSaver> wms(new Widelands::MapSaver(*tmp_fs_, *this));
237+ wms->save();
238+
239+ // swap map fs
240+ std::unique_ptr<FileSystem> mapfs(tmp_fs_->make_sub_file_system("."));
241+ map_.swap_filesystem(mapfs);
242+ mapfs.reset();
243+
244+ // This is just a convenience hack:
245+ // If tmp_fs_ is a zip filesystem then - because of the way zip filesystems are currently implemented -
246+ // the file is still in zip mode right now, which means that the file isn't finalized yet, i.e.,
247+ // not even a valid zip file until zip mode ends. To force ending the zip mode (thus finalizing the file)
248+ // we simply perform a (otherwise useless) filesystem request.
249+ // It's not strictly necessary, but this way we get a valid zip file immediately istead of
250+ // at some unkown later point (when an unzip operation happens or a filesystem object destructs).
251+ tmp_fs_->file_exists("binary");
252 }
253
254 void EditorGameBase::think() {
255@@ -196,6 +274,16 @@
256 * graphics are loaded.
257 */
258 void EditorGameBase::postload() {
259+ if (map_.filesystem()) {
260+ // save map data to temporary file and reassign map fs
261+ try {
262+ create_tempfile_and_save_mapdata(FileSystem::ZIP);
263+ } catch (const WException& e) {
264+ log("EditorGameBase::postload: saving map to temporary file failed: %s", e.what());
265+ throw;
266+ }
267+ }
268+
269 // Postload tribes
270 assert(tribes_);
271 tribes_->postload();
272@@ -411,6 +499,8 @@
273 player_manager_->cleanup();
274
275 map_.cleanup();
276+
277+ delete_tempfile();
278 }
279
280 void EditorGameBase::set_road(const FCoords& f, uint8_t const direction, uint8_t const roadtype) {
281
282=== modified file 'src/logic/editor_game_base.h'
283--- src/logic/editor_game_base.h 2018-04-07 16:59:00 +0000
284+++ src/logic/editor_game_base.h 2018-11-09 08:01:52 +0000
285@@ -255,6 +255,15 @@
286 std::unique_ptr<InteractiveBase> ibase_;
287 Map map_;
288
289+ /// Even after a map is fully loaded, some static data (images, scripts)
290+ /// will still be read from a filesystem whenever a map/game is saved.
291+ /// To avoid potential filesystem conflicts when (pre)loading/saving/deleting
292+ /// map/game files (and to avoid having to deal with this in many different places)
293+ /// a temporary file (in a special dir) is created for such data.
294+ std::unique_ptr<FileSystem> tmp_fs_;
295+ void delete_tempfile();
296+ void create_tempfile_and_save_mapdata(FileSystem::Type type);
297+
298 DISALLOW_COPY_AND_ASSIGN(EditorGameBase);
299 };
300
301
302=== modified file 'src/logic/filesystem_constants.h'
303--- src/logic/filesystem_constants.h 2018-04-22 16:01:32 +0000
304+++ src/logic/filesystem_constants.h 2018-11-09 08:01:52 +0000
305@@ -38,6 +38,13 @@
306 const std::string kS2MapExtension1 = ".swd";
307 const std::string kS2MapExtension2 = ".wld";
308
309+/// Filesystem names for temp files holding static data that needs to be accessible via filesystem
310+/// Kept in a separate dir to avoid filesystem conflicts
311+const std::string kTempFileDir = "temp";
312+const std::string kTempFileExtension = ".tmp";
313+// We delete (accidentally remaining) temp files older than a week
314+constexpr double kTempFilesKeepAroundTime = 7 * 24 * 60 * 60;
315+
316 /// Filesystem names and timeouts for replays
317 const std::string kReplayDir = "replays";
318 const std::string kReplayExtension = ".wrpl";
319
320=== modified file 'src/wlapplication.cc'
321--- src/wlapplication.cc 2018-10-09 17:09:34 +0000
322+++ src/wlapplication.cc 2018-11-09 08:01:52 +0000
323@@ -339,6 +339,7 @@
324 changedir_on_mac();
325 cleanup_replays();
326 cleanup_ai_files();
327+ cleanup_temp_files();
328
329 Section& config = g_options.pull_section("global");
330
331@@ -1498,6 +1499,21 @@
332 }
333 }
334
335+/**
336+ * Delete old temp files that might still lurk around (game crashes etc.)
337+ */
338+void WLApplication::cleanup_temp_files() {
339+ for (const std::string& filename :
340+ filter(g_fs->list_directory(kTempFileDir), [](const std::string& fn) {
341+ return boost::ends_with(fn, kTempFileExtension);
342+ })) {
343+ if (is_autogenerated_and_expired(filename, kTempFilesKeepAroundTime)) {
344+ log("Deleting old temp file: %s\n", filename.c_str());
345+ g_fs->fs_unlink(filename);
346+ }
347+ }
348+}
349+
350 bool WLApplication::redirect_output(std::string path) {
351 if (path.empty()) {
352 #ifdef _WIN32
353
354=== modified file 'src/wlapplication.h'
355--- src/wlapplication.h 2018-04-07 16:59:00 +0000
356+++ src/wlapplication.h 2018-11-09 08:01:52 +0000
357@@ -216,6 +216,8 @@
358
359 void cleanup_ai_files();
360
361+ void cleanup_temp_files();
362+
363 bool redirect_output(std::string path = "");
364
365 // Handle the given pressed key. Returns true when key was

Subscribers

People subscribed via source and target branches

to status/vote changes: