From a4c925a4457c31badfc7ef3dab59ec955fc12af7 Mon Sep 17 00:00:00 2001 From: Hanif Koh Date: Thu, 24 Sep 2026 23:52:29 +0800 Subject: [PATCH] Let an Explicit Config Save Bypass the Back-Off and Lock the Scan's Failure Cleanup The ten-second back-off after a failed config write sat inside save() itself, so the save on exit could return without writing and lose the session's changes; it gates only the idle-time save now, through save_due(), and an explicit save always tries. The preset scan's failure handlers removed a broken preset and its .info after the per-file guard had ended, so another instance's fresh copy of that file could be deleted from under it; they take the guard too. A lock that is acquired ends the cool-down a timeout started, rather than letting every guard for the rest of it proceed unlocked whenever the lock is momentarily held. The lock file's identity is checked on every acquisition, one stat, since a handle to a replaced file locks nothing anyone else can see. Write access on Windows is probed with an open for writing, which sees ACLs where _waccess sees only the read-only attribute, and a temporary name too long for the filesystem falls back to an in-place write for a new file as well as an existing one. The lock tests restore the knobs they change however they end. --- src/libslic3r/AppConfig.cpp | 4 ---- src/libslic3r/AppConfig.hpp | 6 ++++-- src/libslic3r/InstanceLock.cpp | 3 +-- src/libslic3r/InstanceLock.hpp | 6 ++++-- src/libslic3r/Preset.cpp | 2 ++ src/libslic3r/utils.cpp | 11 ++++++++--- src/slic3r/GUI/GUI_App.cpp | 2 +- tests/libslic3r/test_instance_lock.cpp | 17 +++++++++++------ 8 files changed, 31 insertions(+), 20 deletions(-) diff --git a/src/libslic3r/AppConfig.cpp b/src/libslic3r/AppConfig.cpp index b6d0515afe..680583f438 100644 --- a/src/libslic3r/AppConfig.cpp +++ b/src/libslic3r/AppConfig.cpp @@ -987,8 +987,6 @@ void AppConfig::save() BOOST_LOG_TRIVIAL(fatal) << "Calling AppConfig::save() from a worker thread!"; throw CriticalException("Calling AppConfig::save() from a worker thread!"); } - if (std::chrono::steady_clock::now() < m_retry_save_at) - return; // The config is first written to a file with a PID suffix and then moved // to avoid race conditions with multiple instances of Slic3r @@ -1271,8 +1269,6 @@ void AppConfig::save() { if (! is_main_thread_active()) throw CriticalException("Calling AppConfig::save() from a worker thread!"); - if (std::chrono::steady_clock::now() < m_retry_save_at) - return; // The config is first written to a file with a PID suffix and then moved // to avoid race conditions with multiple instances of Slic3r diff --git a/src/libslic3r/AppConfig.hpp b/src/libslic3r/AppConfig.hpp index f3199c272f..5d2ecd9d16 100644 --- a/src/libslic3r/AppConfig.hpp +++ b/src/libslic3r/AppConfig.hpp @@ -131,6 +131,9 @@ public: // Does this config need to be saved? bool dirty() const { return m_dirty; } + // False for ten seconds after a failed write, so the idle handler does not + // repeat a hopeless attempt on every event; an explicit save() always tries. + bool save_due() const { return std::chrono::steady_clock::now() >= m_retry_save_at; } void set_dirty() { m_dirty = true; } @@ -458,8 +461,7 @@ private: bool write_config_file(const std::string &path, std::string body, const std::string &checksum_source); bool m_dirty; - // After a failed write, save() does nothing until this point, so the idle - // handler does not repeat the attempt on every event. + // After a failed write, save_due() is false until this point. std::chrono::steady_clock::time_point m_retry_save_at{}; // Original version found in the ini file before it was overwritten Semver m_orig_version; diff --git a/src/libslic3r/InstanceLock.cpp b/src/libslic3r/InstanceLock.cpp index 9fec7c261c..f030caef19 100644 --- a/src/libslic3r/InstanceLock.cpp +++ b/src/libslic3r/InstanceLock.cpp @@ -28,8 +28,6 @@ struct InstanceLock::Slot std::unique_ptr 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. - // Checked at most every few seconds, since a preset scan takes one guard - // per file. std::string identity; std::chrono::steady_clock::time_point identity_checked_at{}; int depth{0}; @@ -111,6 +109,7 @@ 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->skip_waiting_until = {}; break; } } catch (const std::exception &e) { diff --git a/src/libslic3r/InstanceLock.hpp b/src/libslic3r/InstanceLock.hpp index 13ef879c73..9d8d3b795a 100644 --- a/src/libslic3r/InstanceLock.hpp +++ b/src/libslic3r/InstanceLock.hpp @@ -41,8 +41,10 @@ public: // 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. Mutable so tests can shorten it. - static inline std::chrono::milliseconds identity_check_interval{5000}; + // the one it opened: on every acquisition by default, one stat, since a + // handle to a replaced file would lock nothing anyone else can see. + // Mutable so tests can change it. + static inline std::chrono::milliseconds identity_check_interval{0}; // An empty path makes the guard a no-op. explicit InstanceLock(const std::string &lock_file_path, std::chrono::milliseconds timeout = default_timeout); diff --git a/src/libslic3r/Preset.cpp b/src/libslic3r/Preset.cpp index 7042db1cdb..a1773cd805 100644 --- a/src/libslic3r/Preset.cpp +++ b/src/libslic3r/Preset.cpp @@ -1866,6 +1866,7 @@ 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); @@ -1877,6 +1878,7 @@ void PresetCollection::load_presets( ++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); diff --git a/src/libslic3r/utils.cpp b/src/libslic3r/utils.cpp index d2d012c3cf..bd04799854 100644 --- a/src/libslic3r/utils.cpp +++ b/src/libslic3r/utils.cpp @@ -740,7 +740,12 @@ std::error_code rename_file(const std::string &from, const std::string &to) static bool is_writable(const std::string &path) { #ifdef _WIN32 - return ::_waccess(boost::nowide::widen(path).c_str(), 2) == 0; + // _waccess() sees only the read-only attribute; an open for writing sees ACLs too. + HANDLE handle = ::CreateFileW(boost::nowide::widen(path).c_str(), GENERIC_WRITE, FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, nullptr, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, nullptr); + if (handle == INVALID_HANDLE_VALUE) + return false; + ::CloseHandle(handle); + return true; #else return ::access(path.c_str(), W_OK) == 0; #endif @@ -784,9 +789,9 @@ std::error_code write_file_atomically(const std::string &path, std::initializer_ boost::nowide::remove(tmp_path.c_str()); // A directory that allows writing the file but not creating one beside // it, or a name the longer temporary pushes past the path limit. - const bool only_the_temporary_failed = ec == std::errc::permission_denied || ec == std::errc::no_such_file_or_directory || + const bool only_the_temporary_failed = (target_exists && (ec == std::errc::permission_denied || ec == std::errc::no_such_file_or_directory)) || ec == std::errc::filename_too_long; - if (target_exists && only_the_temporary_failed) { + if (only_the_temporary_failed) { BOOST_LOG_TRIVIAL(warning) << "Cannot create a temporary beside " << path << " (" << ec.message() << "); writing in place"; return write_whole_file(path, chunks, binary); } diff --git a/src/slic3r/GUI/GUI_App.cpp b/src/slic3r/GUI/GUI_App.cpp index 7a91487acf..327953ef70 100644 --- a/src/slic3r/GUI/GUI_App.cpp +++ b/src/slic3r/GUI/GUI_App.cpp @@ -3532,7 +3532,7 @@ bool GUI_App::on_init_inner() update_publish_status(); } - if (m_post_initialized && app_config->dirty()) + if (m_post_initialized && app_config->dirty() && app_config->save_due()) app_config->save(); }); diff --git a/tests/libslic3r/test_instance_lock.cpp b/tests/libslic3r/test_instance_lock.cpp index 36ca30670a..3cd3a0a7d9 100644 --- a/tests/libslic3r/test_instance_lock.cpp +++ b/tests/libslic3r/test_instance_lock.cpp @@ -18,6 +18,15 @@ using namespace Slic3r; using namespace std::chrono_literals; +// Sets a process-wide knob for one test and restores it however the test ends. +template struct ScopedStaticValue +{ + T &ref; + T saved; + ScopedStaticValue(T &ref, T value) : ref(ref), saved(ref) { ref = value; } + ~ScopedStaticValue() { ref = saved; } +}; + TEST_CASE("InstanceLock creates its lock file and holds it for the guard's scope", "[InstanceLock]") { ScopedTemporaryFile lock_file(".lock"); @@ -67,8 +76,7 @@ TEST_CASE("InstanceLock retries a lock file it could not open once the cool-down { ScopedTemporaryDir dir; const std::string path = (dir.path() / "later" / "shared.lock").string(); - const auto saved_cooldown = InstanceLock::cooldown; - InstanceLock::cooldown = 300ms; + ScopedStaticValue cooldown(InstanceLock::cooldown, 300ms); bool before_dir, during_cooldown, after_cooldown; const auto started = std::chrono::steady_clock::now(); @@ -87,7 +95,6 @@ TEST_CASE("InstanceLock retries a lock file it could not open once the cool-down InstanceLock lock(path, 100ms); after_cooldown = lock.locked(); } - InstanceLock::cooldown = saved_cooldown; REQUIRE_FALSE(before_dir); // A loaded runner may take longer than the cool-down to get here; then the @@ -101,8 +108,7 @@ TEST_CASE("InstanceLock reopens a lock file that was replaced on disk", "[Instan { ScopedTemporaryFile lock_file(".lock"); const std::string path = lock_file.string(); - const auto saved_interval = InstanceLock::identity_check_interval; - InstanceLock::identity_check_interval = 0ms; + ScopedStaticValue interval(InstanceLock::identity_check_interval, 0ms); { InstanceLock lock(path); REQUIRE(lock.locked()); @@ -110,7 +116,6 @@ TEST_CASE("InstanceLock reopens a lock file that was replaced on disk", "[Instan boost::filesystem::remove(path); InstanceLock lock(path); - InstanceLock::identity_check_interval = saved_interval; 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