Open the Lock File Per Guard and Keep Directory Removal Out of the Lock

The lock file stayed open for the life of the process, which pinned the
data dir on Windows so the preset tests leaked a directory each, and
asked for a check that the file behind the path was still the one
opened. The outermost guard opens the file and closes it when it goes,
so whatever is at the path is what gets locked and nothing stays open
between saves; the identity check and its interval are gone with it.

Removing a bundle or a user folder held the lock for the whole tree, and
the wait for the in-process mutex is bounded only by the longest
critical section, so a save on the GUI thread could wait for a tree of
files to go on a slow share. The tree is renamed aside under the lock in
one step and removed afterwards. A bundle import extracts into a
per-process folder, so two instances importing at once do not clear
each other's extraction. A lock file that cannot be opened or locked
backs off for longer with each failure, like a timed-out wait does; the
preset scan's three removal sites share one helper; a restore that fails
after a refused rename names where the previous content went; and the
warnings about a set-aside file and an in-place fallback are logged once
per file.

src/libslic3r/InstanceLock.cpp

@@ -3,6 +3,7 @@
 #include <algorithm>
 #include <map>
 #include <memory>
+#include <system_error>
 #include <thread>
 
 #include <boost/filesystem.hpp>
@@ -17,9 +18,6 @@
 #include <sys/file.h>
 #include <unistd.h>
 #endif
-#include <system_error>
-
-#include "Utils.hpp"
 
 namespace Slic3r {
 
@@ -56,16 +54,14 @@ private:
 
 // One slot per lock file, shared by every guard in the process: one lock
 // object per path behind a mutex is what makes the guard re-entrant and safe
-// to use from the preset sync thread and the GUI thread at once.
+// to use from the preset sync thread and the GUI thread at once. The lock
+// file is opened by the outermost guard and closed when it goes, so the file
+// is never held open between guards: whatever is at the path is what gets
+// locked, and a data dir can be removed once nothing is saving into it.
 struct InstanceLock::Slot
 {
     std::recursive_mutex mutex;
-    // Absent until the lock file has been created and opened.
     std::unique_ptr<NativeFileLock> file_lock;
-    // What the lock file was when it was opened; a different answer means the
-    // path was unlinked or replaced and the open handle locks a dead file.
-    std::string identity;
-    std::chrono::steady_clock::time_point identity_checked_at{};
     int  depth{0};
     bool file_locked{false};
     // After a timed-out wait, guards skip waiting until this point; each wait
@@ -73,14 +69,14 @@ struct InstanceLock::Slot
     std::chrono::steady_clock::time_point skip_waiting_until{};
     int consecutive_timeouts{0};
     // After a failed open or a failing lock call, guards skip the file lock
-    // entirely until this point.
+    // entirely until this point, again doubling for failures in a row.
     std::chrono::steady_clock::time_point retry_at{};
+    int consecutive_failures{0};
 };
 
 InstanceLock::Slot &InstanceLock::slot_for(const std::string &lock_file_path)
 {
-    // Never freed: the slots keep the lock files open for as long as anything
-    // in the process may still save, including during static destruction.
+    // Never freed: a save during static destruction still needs its slot.
     static auto *registry_mutex = new std::mutex();
     static auto *registry       = new std::map<std::string, std::unique_ptr<Slot>>();
 
@@ -91,6 +87,11 @@ InstanceLock::Slot &InstanceLock::slot_for(const std::string &lock_file_path)
     return *slot;
 }
 
+static std::chrono::milliseconds backoff_for(int failures_in_a_row, std::chrono::milliseconds base)
+{
+    return std::min(base * (1 << std::min(failures_in_a_row, 5)), std::chrono::milliseconds(std::chrono::minutes(5)));
+}
+
 // Creates the lock file if needed and opens it, or starts the cool-down.
 // Called with the slot mutex held.
 bool InstanceLock::open_lock_file(Slot &slot, const std::string &lock_file_path)
@@ -108,14 +109,13 @@ bool InstanceLock::open_lock_file(Slot &slot, const std::string &lock_file_path)
 #else
         slot.file_lock = std::make_unique<NativeFileLock>(lock_file_path.c_str());
 #endif
-        slot.identity            = file_identity(lock_file_path);
-        slot.identity_checked_at = std::chrono::steady_clock::now();
         return true;
     } catch (const std::exception &e) {
-        slot.retry_at = std::chrono::steady_clock::now() + cooldown;
+        const auto backoff = backoff_for(slot.consecutive_failures ++, cooldown);
+        slot.retry_at      = std::chrono::steady_clock::now() + backoff;
         BOOST_LOG_TRIVIAL(warning) << "Cannot open lock file " << lock_file_path << ": " << e.what()
                                    << " (check its owner and permissions); other instances are not excluded from writing for the next "
-                                   << cooldown.count() << " ms";
+                                   << backoff.count() << " ms";
         return false;
     }
 }
@@ -127,17 +127,7 @@ InstanceLock::InstanceLock(const std::string &lock_file_path, std::chrono::milli
     m_slot       = &slot_for(lock_file_path);
     m_slot_guard = std::unique_lock<std::recursive_mutex>(m_slot->mutex);
     const auto now = std::chrono::steady_clock::now();
-    // A handle to a lock file that was deleted or recreated since it was opened
-    // would lock a file no other instance can see.
-    if (m_slot->depth == 0 && m_slot->file_lock && now - m_slot->identity_checked_at >= identity_check_interval) {
-        m_slot->identity_checked_at = now;
-        if (file_identity(lock_file_path) != m_slot->identity) {
-            BOOST_LOG_TRIVIAL(info) << "Lock file " << lock_file_path << " was replaced; reopening it";
-            m_slot->file_lock.reset();
-        }
-    }
-    if (m_slot->depth == 0 && now >= m_slot->retry_at &&
-        (m_slot->file_lock || open_lock_file(*m_slot, lock_file_path))) {
+    if (m_slot->depth == 0 && now >= m_slot->retry_at && open_lock_file(*m_slot, lock_file_path)) {
         const bool wait     = now >= m_slot->skip_waiting_until;
         const auto deadline = now + timeout;
         for (;;) {
@@ -145,20 +135,21 @@ InstanceLock::InstanceLock(const std::string &lock_file_path, std::chrono::milli
                 if (m_slot->file_lock->try_lock()) {
                     m_slot->file_locked          = true;
                     m_slot->consecutive_timeouts = 0;
+                    m_slot->consecutive_failures = 0;
                     m_slot->skip_waiting_until   = {};
                     break;
                 }
             } catch (const std::exception &e) {
-                m_slot->retry_at = std::chrono::steady_clock::now() + cooldown;
+                const auto backoff = backoff_for(m_slot->consecutive_failures ++, cooldown);
+                m_slot->retry_at   = std::chrono::steady_clock::now() + backoff;
                 BOOST_LOG_TRIVIAL(warning) << "Cannot lock " << lock_file_path << ": " << e.what()
-                                           << "; proceeding without the lock for the next " << cooldown.count() << " ms";
+                                           << "; proceeding without the lock for the next " << backoff.count() << " ms";
                 break;
             }
             if (! wait)
                 break;
             if (std::chrono::steady_clock::now() >= deadline) {
-                const auto backoff = std::min(cooldown * (1 << std::min(m_slot->consecutive_timeouts, 5)), std::chrono::milliseconds(std::chrono::minutes(5)));
-                ++ m_slot->consecutive_timeouts;
+                const auto backoff = backoff_for(m_slot->consecutive_timeouts ++, cooldown);
                 m_slot->skip_waiting_until = deadline + backoff;
                 BOOST_LOG_TRIVIAL(warning) << "Another instance has held " << lock_file_path << " for over "
                                            << timeout.count() << " ms; proceeding without the lock for the next "
@@ -167,6 +158,8 @@ InstanceLock::InstanceLock(const std::string &lock_file_path, std::chrono::milli
             }
             std::this_thread::sleep_for(std::chrono::milliseconds(5));
         }
+        if (! m_slot->file_locked)
+            m_slot->file_lock.reset();
     }
     // Counted last, so a throw above leaves the slot exactly as it was found.
     ++ m_slot->depth;
@@ -177,13 +170,16 @@ InstanceLock::~InstanceLock()
 {
     if (m_slot == nullptr)
         return;
-    if (-- m_slot->depth == 0 && m_slot->file_locked) {
-        try {
-            m_slot->file_lock->unlock();
-        } catch (const std::exception &e) {
-            BOOST_LOG_TRIVIAL(warning) << "Cannot unlock instance lock: " << e.what();
+    if (-- m_slot->depth == 0 && m_slot->file_lock) {
+        if (m_slot->file_locked) {
+            try {
+                m_slot->file_lock->unlock();
+            } catch (const std::exception &e) {
+                BOOST_LOG_TRIVIAL(warning) << "Cannot unlock instance lock: " << e.what();
+            }
+            m_slot->file_locked = false;
         }
-        m_slot->file_locked = false;
+        m_slot->file_lock.reset();
     }
 }
 

src/libslic3r/InstanceLock.hpp

@@ -11,11 +11,14 @@ namespace Slic3r {
 // of this process are serialised through a recursive mutex, other processes
 // through an advisory OS file lock on `lock_file_path` (flock on POSIX, tied to
 // the guard's own open file so nothing else in the process can drop it by
-// opening and closing the file; LockFileEx on Windows), created on first use
-// and kept: the lock state lives in the kernel on the open file, so deleting
-// the file on release would let a third instance lock a fresh file while the
-// second still holds the old one. The OS releases the lock when its holder
-// exits, so a crashed instance never leaves a stale lock behind.
+// opening and closing the file; LockFileEx on Windows). The file is created on
+// first use and kept, since the lock state lives in the kernel on the open
+// file and deleting the file on release would let a third instance lock a
+// fresh file while the second still holds the old one; it is opened by the
+// outermost guard and closed when that guard goes, so whatever is at the path
+// is what gets locked and nothing stays open between saves. The OS releases
+// the lock when its holder exits, so a crashed instance never leaves a stale
+// lock behind.
 //
 // The lock is best effort: when the lock file cannot be created, or another
 // instance still holds it after `timeout` (a second by default), the guard keeps only the in-process
@@ -26,7 +29,10 @@ namespace Slic3r {
 // pays the wait once rather than once per file and a holder that never lets go
 // does not cost a stall every cool-down for good; a lock file that could
 // not be opened, or a lock call that fails outright (a share without a lock
-// service), is likewise retried only after `cooldown`.
+// service), is likewise retried only after `cooldown`, doubling the same way.
+// A wait for the in-process mutex is bounded only by the longest critical
+// section, which is why every guard covers a few file operations and nothing
+// slower, such as removing a directory tree.
 //
 // Lock order: the preset collection mutex may be held when a guard is taken
 // (set_sync_info_and_save() calls save_info() under it), never the reverse.
@@ -42,11 +48,6 @@ public:
     // How long a lock file is left alone after a timed-out wait, a failed open
     // or a failing lock call. Mutable so tests can shorten it.
     static inline std::chrono::milliseconds cooldown{10000};
-    // How often a guard re-checks that the lock file behind the path is still
-    // the one it opened, since a handle to a replaced file would lock nothing
-    // anyone else can see: a stat once a second bounds that window while a scan
-    // of hundreds of presets pays for it once. Mutable so tests can change it.
-    static inline std::chrono::milliseconds identity_check_interval{1000};
 
     // An empty path makes the guard a no-op.
     explicit InstanceLock(const std::string &lock_file_path, std::chrono::milliseconds timeout = default_timeout);

src/libslic3r/Preset.cpp

@@ -112,6 +112,38 @@ std::string user_presets_lock_path(bool read_only)
     return read_only || data_dir().empty() ? std::string() : (fs::path(data_dir()) / (PRESET_USER_DIR ".lock")).string();
 }
 
+// Removes a preset file the scan could not load, and its .info, under the lock.
+static void remove_preset_files(const std::string &preset_file, bool read_only)
+{
+    if (read_only)
+        return;
+    InstanceLock instance_lock(user_presets_lock_path());
+    boost::system::error_code ec;
+    fs::path file_path(preset_file);
+    fs::remove(file_path, ec);
+    file_path.replace_extension(".info");
+    fs::remove(file_path, ec);
+}
+
+void remove_directory_tree_locked(const boost::filesystem::path &dir)
+{
+    boost::system::error_code ec;
+    fs::path doomed = dir;
+    doomed += "." + std::to_string(get_current_pid()) + ".removing";
+    {
+        InstanceLock instance_lock(user_presets_lock_path());
+        if (! fs::exists(dir, ec))
+            return;
+        fs::rename(dir, doomed, ec);
+        if (ec) {
+            // Cannot be set aside: the slow way, still under the lock.
+            fs::remove_all(dir, ec);
+            return;
+        }
+    }
+    fs::remove_all(doomed, ec);
+}
+
 std::string get_preset_bare_name(const std::string &canonical_name)
 {
     const auto pos = canonical_name.find_last_of('/');
@@ -1770,12 +1802,7 @@ void PresetCollection::load_presets(
                         //ConfigSubstitutions config_substitutions = config.load_from_ini(preset.file, substitution_rule);
                         config_substitutions = config.load_from_json(preset.file, substitution_rule, key_values, reason);
                         if (!reason.empty()) {
-                            fs::path file_path(preset.file);
-                            if (!read_only && fs::exists(file_path))
-                                fs::remove(file_path);
-                            file_path.replace_extension(".info");
-                            if (!read_only && fs::exists(file_path))
-                                fs::remove(file_path);
+                            remove_preset_files(preset.file, read_only);
                             BOOST_LOG_TRIVIAL(error) << boost::format("parse config %1% failed")%preset.file;
                             ++m_errors;
                             continue;
@@ -1866,25 +1893,13 @@ void PresetCollection::load_presets(
                 } catch (const std::ifstream::failure &err) {
                     ++m_errors;
                     BOOST_LOG_TRIVIAL(error) << boost::format("The user-config cannot be loaded: %1%. Reason: %2%")%preset.file %err.what();
-                    InstanceLock instance_lock(user_presets_lock_path(read_only));
-                    fs::path file_path(preset.file);
-                    if (!read_only && fs::exists(file_path))
-                        fs::remove(file_path);
-                    file_path.replace_extension(".info");
-                    if (!read_only && fs::exists(file_path))
-                        fs::remove(file_path);
+                    remove_preset_files(preset.file, read_only);
                     //throw Slic3r::RuntimeError(std::string("The selected preset cannot be loaded: ") + preset.file + "\n\tReason: " + err.what());
                 } catch (const std::runtime_error &err) {
                     ++m_errors;
                     BOOST_LOG_TRIVIAL(error) << boost::format("Failed loading the user-config file: %1%. Reason: %2%")%preset.file %err.what();
                     //throw Slic3r::RuntimeError(std::string("Failed loading the preset file: ") + preset.file + "\n\tReason: " + err.what());
-                    InstanceLock instance_lock(user_presets_lock_path(read_only));
-                    fs::path file_path(preset.file);
-                    if (!read_only && fs::exists(file_path))
-                        fs::remove(file_path);
-                    file_path.replace_extension(".info");
-                    if (!read_only && fs::exists(file_path))
-                        fs::remove(file_path);
+                    remove_preset_files(preset.file, read_only);
                 }
 
                 if (preset_loaded_fn != nullptr)

src/libslic3r/Preset.hpp

@@ -490,6 +490,12 @@ std::string get_preset_bare_name(const std::string &canonical_name);
 // many jobs on one data dir.
 std::string user_presets_lock_path(bool read_only = false);
 
+// Removes a directory tree under the user preset lock without holding the lock
+// for the removal itself: the tree is renamed aside under the lock, in one
+// step, and deleted afterwards, so a save waiting on the lock waits
+// milliseconds rather than for a tree of files to go.
+void remove_directory_tree_locked(const boost::filesystem::path &dir);
+
 // Resolve an origin from a directory path when the caller passes Kind::Auto.
 PresetOrigin detect_origin_from_path(const boost::filesystem::path &path, const PresetOrigin &explicit_origin = PresetOrigin());
 

src/libslic3r/PresetBundle.cpp

@@ -1619,7 +1619,8 @@ PresetsConfigSubstitutions PresetBundle::import_presets(std::vector<std::string>
             if (ec) BOOST_LOG_TRIVIAL(error) << __FUNCTION__ << " create directory failed: " << ec.message();
             //create temp folder
             //std::string user_default_temp_dir = data_dir() + "/" + PRESET_USER_DIR + "/" + DEFAULT_USER_FOLDER_NAME + "/" + "temp";
-            fs::path temp_folder(configs_folder / "temp");
+            // Per process, so two instances importing at once do not clear each other's extraction.
+            fs::path temp_folder(configs_folder / ("temp_" + std::to_string(get_current_pid())));
             std::string user_default_temp_dir = temp_folder.make_preferred().string();
             if (fs::exists(temp_folder)) fs::remove_all(temp_folder);
             fs::create_directory(temp_folder, ec);
@@ -2241,11 +2242,7 @@ void PresetBundle::remove_user_presets_directory(const std::string preset_folder
         return;
     }
     BOOST_LOG_TRIVIAL(debug) << __FUNCTION__ << boost::format(" enter, delete directory : %1%") % dir_user_presets;
-    InstanceLock instance_lock(user_presets_lock_path());
-    fs::path folder(dir_user_presets);
-    if (fs::exists(folder)) {
-        fs::remove_all(folder);
-    }
+    remove_directory_tree_locked(fs::path(dir_user_presets));
 }
 
 void PresetBundle::update_system_preset_setting_ids(std::map<std::string, std::map<std::string, std::string>>& system_presets)

src/libslic3r/Utils.hpp

@@ -246,9 +246,6 @@ inline std::error_code write_file_atomically(const std::string &path, const std:
 // may be a last copy or a since-deleted file and so stay for the user. Runs at
 // most once an hour per directory and prefix. Returns how many were removed.
 extern size_t remove_stale_temp_files(const boost::filesystem::path &dir, const std::string &name_prefix = std::string());
-// Names the file object behind `path` (device and inode, or volume and file index),
-// for noticing that a path was unlinked and recreated. Empty when it cannot be read.
-extern std::string file_identity(const std::string &path);
 
 enum CopyFileResult {
 	SUCCESS = 0,

src/libslic3r/utils.cpp

@@ -12,6 +12,7 @@
 #include <cerrno>
 #include <chrono>
 #include <map>
+#include <set>
 #include <mutex>
 #include <thread>
 #include <cstring>
@@ -458,7 +459,7 @@ boost::filesystem::path get_log_file_name()
 
 #ifdef _WIN32
 // The following helpers are borrowed from the LLVM project https://github.com/llvm
-// Names a file object by volume and file index, the same identity file_identity() reports.
+// Names a file object by volume and file index.
 static std::string file_identity_of(const BY_HANDLE_FILE_INFORMATION &info)
 {
 	return std::to_string(info.dwVolumeSerialNumber) + ":" + std::to_string((static_cast<uint64_t>(info.nFileIndexHigh) << 32) | info.nFileIndexLow);
@@ -735,7 +736,8 @@ std::error_code rename_file(const std::string &from, const std::string &to)
 				boost::nowide::remove(aside.c_str());
 				return {};
 			}
-			boost::nowide::rename(aside.c_str(), to.c_str());
+			if (boost::nowide::rename(aside.c_str(), to.c_str()) != 0)
+				BOOST_LOG_TRIVIAL(error) << "Could not put " << to << " back after a failed replace; its previous content is at " << aside;
 		}
 	}
 	return std::make_error_code(static_cast<std::errc>(err));
@@ -819,7 +821,19 @@ std::error_code write_file_atomically(const std::string &path, std::initializer_
 		// or a mount that cannot replace a file at all. Losing the save is worse
 		// than a reader seeing a partial file, so write in place the way this
 		// used to work before the atomic path existed.
-		BOOST_LOG_TRIVIAL(warning) << "Cannot replace " << path << " (" << ec.message() << "); writing in place";
+		// Once per file at warning level, so a mount that never replaces does
+		// not fill the log, and the loss of the atomic path is still on record.
+		static std::mutex warned_mutex;
+		static std::set<std::string> warned;
+		bool first;
+		{
+			std::lock_guard<std::mutex> guard(warned_mutex);
+			first = warned.insert(path).second;
+		}
+		if (first)
+			BOOST_LOG_TRIVIAL(warning) << "Cannot replace " << path << " (" << ec.message() << "); writing in place, here and for later saves of this file";
+		else
+			BOOST_LOG_TRIVIAL(info) << "Cannot replace " << path << " (" << ec.message() << "); writing in place";
 		return write_whole_file(path, chunks, binary);
 	}
 	// Crash leftovers of earlier writes into this directory, for the files the
@@ -830,27 +844,6 @@ std::error_code write_file_atomically(const std::string &path, std::initializer_
 	return {};
 }
 
-std::string file_identity(const std::string &path)
-{
-#ifdef _WIN32
-	const std::wstring wide = boost::nowide::widen(path);
-	HANDLE handle = ::CreateFileW(wide.c_str(), 0, FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, nullptr, OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, nullptr);
-	if (handle == INVALID_HANDLE_VALUE)
-		return {};
-	BY_HANDLE_FILE_INFORMATION info;
-	const bool ok = ::GetFileInformationByHandle(handle, &info) != 0;
-	::CloseHandle(handle);
-	if (! ok)
-		return {};
-	return file_identity_of(info);
-#else
-	struct stat st;
-	if (::stat(path.c_str(), &st) != 0)
-		return {};
-	return std::to_string(static_cast<unsigned long long>(st.st_dev)) + ":" + std::to_string(static_cast<unsigned long long>(st.st_ino));
-#endif
-}
-
 size_t remove_stale_temp_files(const boost::filesystem::path &dir, const std::string &name_prefix)
 {
 	// Once an hour per directory: a batch of saves or a load followed by a
@@ -910,7 +903,11 @@ size_t remove_stale_temp_files(const boost::filesystem::path &dir, const std::st
 			// A target rename_file() moved aside and never put back or removed.
 			// It may be the last copy of a file, or a file since deleted on
 			// purpose; neither removing nor restoring it is safe unasked.
-			BOOST_LOG_TRIVIAL(warning) << it->path() << " was set aside by an interrupted save; restore or delete it by hand";
+			static std::mutex warned_mutex;
+			static std::set<std::string> warned;
+			std::lock_guard<std::mutex> guard(warned_mutex);
+			if (warned.insert(it->path().string()).second)
+				BOOST_LOG_TRIVIAL(warning) << it->path() << " was set aside by an interrupted save; restore or delete it by hand";
 			continue;
 		}
 		if (boost::filesystem::remove(it->path(), entry_ec)) {

src/slic3r/GUI/GUI_App.cpp

@@ -7624,11 +7624,7 @@ void GUI_App::start_sync_user_preset(bool with_progress_dlg)
 
                             // Delete the bundle folder and bundle
                             fs::path bundle_folder = fs::path(bundle.path.c_str()).parent_path();
-                            boost::system::error_code ec;
-                            {
-                                InstanceLock instance_lock(user_presets_lock_path());
-                                boost::filesystem::remove_all(bundle_folder, ec);
-                            }
+                            remove_directory_tree_locked(bundle_folder);
 
                             preset_bundle->bundles.WriteLock();
                             preset_bundle->bundles.m_bundles.erase(bundle.id);

tests/libslic3r/test_instance_lock.cpp

@@ -109,7 +109,6 @@ TEST_CASE("InstanceLock reopens a lock file that was replaced on disk", "[Instan
 {
     ScopedTemporaryFile lock_file(".lock");
     const std::string   path = lock_file.string();
-    ScopedStaticValue   interval(InstanceLock::identity_check_interval, 0ms);
     {
         InstanceLock lock(path);
         REQUIRE(lock.locked());
@@ -118,9 +117,7 @@ TEST_CASE("InstanceLock reopens a lock file that was replaced on disk", "[Instan
     boost::filesystem::remove(path);
     InstanceLock lock(path);
     REQUIRE(lock.locked());
-    // Only a reopen recreates the file; a guard still holding the unlinked one
-    // would leave the path missing. (The inode number itself may be reused once
-    // the old handle is closed, so it is no proof either way.)
+    // Each outermost guard opens the file afresh, so the deleted path is back.
     REQUIRE(boost::filesystem::exists(path));
 }