From b7102a524bbfd1b428c9693bb6025a35cdc22b4b Mon Sep 17 00:00:00 2001 From: Hanif Koh Date: Thu, 24 Sep 2026 23:22:31 +0800 Subject: [PATCH] Clear the Read-Only Attribute Before Replacing a Config and Back Off After a Failed Save Nothing replaces a read-only file on Windows, neither a rename over it nor an in-place write, so the opt-in that keeps a read-only config saveable clears the attribute first; without that the test for it could not pass there. A failed config write still leaves the flag dirty, as before this change, but the next attempt waits ten seconds, so the idle handler does not repeat a hopeless write on every event. A file moved aside by a refused rename can be the only copy left if the process dies in between, so the sweep puts such a file back when its original is missing rather than removing it, and the directory-only errors that could only come from a directory target no longer trigger the move-aside at all. The sweep after a write runs only under the data dir and at most once an hour per directory, so a batch of saves does not read the directory once per file and an export into a user's folder never reads that folder. A lock holder that never lets go doubles the cool-down for each timeout in a row, up to five minutes, instead of costing a stall every ten seconds for good. --- src/libslic3r/AppConfig.cpp | 8 +++++- src/libslic3r/AppConfig.hpp | 4 +++ src/libslic3r/InstanceLock.cpp | 14 +++++++--- src/libslic3r/InstanceLock.hpp | 6 +++-- src/libslic3r/Utils.hpp | 7 ++--- src/libslic3r/utils.cpp | 49 +++++++++++++++++++++++++++++----- tests/libslic3r/test_utils.cpp | 19 +++++++++++-- 7 files changed, 88 insertions(+), 19 deletions(-) diff --git a/src/libslic3r/AppConfig.cpp b/src/libslic3r/AppConfig.cpp index 2012a72aee..b6d0515afe 100644 --- a/src/libslic3r/AppConfig.cpp +++ b/src/libslic3r/AppConfig.cpp @@ -987,6 +987,8 @@ 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 @@ -1269,6 +1271,8 @@ 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 @@ -1329,9 +1333,11 @@ bool AppConfig::write_config_file(const std::string &path, std::string body, con #endif // The config was always replaced, never written in place, so a read-only one is replaced still. if (const std::error_code ec = write_file_atomically(path, body, false, /*replace_read_only=*/true)) { - BOOST_LOG_TRIVIAL(error) << "Failed to write the configuration " << path << ": " << ec.message(); + BOOST_LOG_TRIVIAL(error) << "Failed to write the configuration " << path << ": " << ec.message() << "; trying again in 10 s"; + m_retry_save_at = std::chrono::steady_clock::now() + std::chrono::seconds(10); return false; } + m_retry_save_at = {}; return true; } diff --git a/src/libslic3r/AppConfig.hpp b/src/libslic3r/AppConfig.hpp index 2bb274b02a..f3199c272f 100644 --- a/src/libslic3r/AppConfig.hpp +++ b/src/libslic3r/AppConfig.hpp @@ -2,6 +2,7 @@ #define slic3r_AppConfig_hpp_ #include +#include #include #include #include "nlohmann/json.hpp" @@ -457,6 +458,9 @@ 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. + 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; // Whether the existing version is before system profiles & configuration updating diff --git a/src/libslic3r/InstanceLock.cpp b/src/libslic3r/InstanceLock.cpp index 22480349f0..9fec7c261c 100644 --- a/src/libslic3r/InstanceLock.cpp +++ b/src/libslic3r/InstanceLock.cpp @@ -1,5 +1,6 @@ #include "InstanceLock.hpp" +#include #include #include #include @@ -33,8 +34,10 @@ struct InstanceLock::Slot 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. + // After a timed-out wait, guards skip waiting until this point; each wait + // that times out in a row doubles the next cool-down, up to a few minutes. 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. std::chrono::steady_clock::time_point retry_at{}; @@ -106,7 +109,8 @@ InstanceLock::InstanceLock(const std::string &lock_file_path, std::chrono::milli for (;;) { try { if (m_slot->file_lock->try_lock()) { - m_slot->file_locked = true; + m_slot->file_locked = true; + m_slot->consecutive_timeouts = 0; break; } } catch (const std::exception &e) { @@ -118,10 +122,12 @@ InstanceLock::InstanceLock(const std::string &lock_file_path, std::chrono::milli if (! wait) break; if (std::chrono::steady_clock::now() >= deadline) { - m_slot->skip_waiting_until = deadline + cooldown; + 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; + 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 " - << cooldown.count() << " ms"; + << backoff.count() << " ms"; break; } std::this_thread::sleep_for(std::chrono::milliseconds(5)); diff --git a/src/libslic3r/InstanceLock.hpp b/src/libslic3r/InstanceLock.hpp index 4de8d5915a..13ef879c73 100644 --- a/src/libslic3r/InstanceLock.hpp +++ b/src/libslic3r/InstanceLock.hpp @@ -19,8 +19,10 @@ namespace Slic3r { // instance still holds it after `timeout` (a second by default), the guard keeps only the in-process // mutex and locked() reports false. Writes then proceed unprotected rather than // letting one hung instance block every other one from saving. After such a -// timeout the same lock file is not waited on again for `cooldown`, so a batch -// of saves pays the wait once rather than once per file; a lock file that could +// timeout the same lock file is not waited on again for `cooldown`, doubling +// for every further timeout in a row up to a few minutes, so a batch of saves +// 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`. // diff --git a/src/libslic3r/Utils.hpp b/src/libslic3r/Utils.hpp index 4689f49cc2..906ab39031 100644 --- a/src/libslic3r/Utils.hpp +++ b/src/libslic3r/Utils.hpp @@ -235,14 +235,15 @@ extern std::error_code rename_file(const std::string &from, const std::string &t // target that is not a regular file (a symlink, device or pipe) is written in // place, since replacing it would change what it is, and so is a target whose // replace the filesystem refuses or beside which no temporary can be created. -// A successful write also removes stale leftovers of earlier writes to the -// same target. +// A successful write into a directory under data_dir() also removes stale +// leftovers there, at most once an hour per directory. extern std::error_code write_file_atomically(const std::string &path, std::initializer_list chunks, bool binary = false, bool replace_read_only = false); inline std::error_code write_file_atomically(const std::string &path, const std::string &content, bool binary = false, bool replace_read_only = false) { return write_file_atomically(path, { std::string_view(content) }, binary, replace_read_only); } // Remove the `...tmp` files a crashed write_file_atomically() and // the `..old` files a crashed rename_file() left in `dir` at least an -// hour ago, only names starting with `name_prefix` when it is given. Returns how +// hour ago, only names starting with `name_prefix` when it is given; an `.old` +// whose `` is missing is the last copy and is put back instead. 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), diff --git a/src/libslic3r/utils.cpp b/src/libslic3r/utils.cpp index ed4f61a92e..d2d012c3cf 100644 --- a/src/libslic3r/utils.cpp +++ b/src/libslic3r/utils.cpp @@ -11,6 +11,8 @@ #include #include #include +#include +#include #include #include #include @@ -718,7 +720,7 @@ std::error_code rename_file(const std::string &from, const std::string &to) // existing target in one step and report it as one of these; move the // target aside and try again there, and put it back if that fails too, so // a refusal that was really about the source never costs the target. - const bool replace_refused = err == EPERM || err == EACCES || err == EEXIST || err == ENOTEMPTY || err == EBUSY || err == ENOTSUP || err == EOPNOTSUPP; + const bool replace_refused = err == EPERM || err == EACCES || err == EBUSY || err == ENOTSUP || err == EOPNOTSUPP; if (replace_refused) { // Named so remove_stale_temp_files() can clear it after a crash in between. const std::string aside = to + "." + std::to_string(get_current_pid()) + ".old"; @@ -765,8 +767,14 @@ std::error_code write_file_atomically(const std::string &path, std::initializer_ return write_whole_file(path, chunks, binary); // Replacing needs only a writable directory, so a file this process may // not write has to be refused here, as the in-place write used to be. - if (target_exists && ! replace_read_only && ! is_writable(path)) - return std::make_error_code(std::errc::permission_denied); + if (target_exists && ! is_writable(path)) { + if (! replace_read_only) + return std::make_error_code(std::errc::permission_denied); +#ifdef _WIN32 + // Nothing replaces a read-only file on Windows, so the attribute goes first. + boost::filesystem::permissions(path, boost::filesystem::add_perms | boost::filesystem::owner_write, bec); +#endif + } // Unique per process and per call, so two threads writing one target // without a lock never share a temporary. @@ -799,10 +807,26 @@ std::error_code write_file_atomically(const std::string &path, std::initializer_ BOOST_LOG_TRIVIAL(warning) << "Cannot replace " << path << " (" << ec.message() << "); writing in place"; return write_whole_file(path, chunks, binary); } - // Crash leftovers of earlier writes to this same file, wherever it lives; - // the name shapes are specific enough that nothing of the user's matches. - const boost::filesystem::path target_path(path); - remove_stale_temp_files(target_path.parent_path(), target_path.filename().string()); + // Crash leftovers of earlier writes into this directory, for the files the + // application owns; once an hour per directory, so a batch of saves does + // not read the directory once per file. + const boost::filesystem::path dir = boost::filesystem::path(path).parent_path(); + if (! data_dir().empty() && boost::algorithm::starts_with(dir.generic_string(), boost::filesystem::path(data_dir()).generic_string())) { + static std::mutex swept_mutex; + static std::map swept_at; + const auto now = std::chrono::steady_clock::now(); + bool sweep = false; + { + std::lock_guard guard(swept_mutex); + auto &last = swept_at[dir.string()]; + if (last == std::chrono::steady_clock::time_point{} || now - last >= std::chrono::hours(1)) { + last = now; + sweep = true; + } + } + if (sweep) + remove_stale_temp_files(dir); + } return {}; } @@ -869,6 +893,17 @@ size_t remove_stale_temp_files(const boost::filesystem::path &dir, const std::st const std::time_t written = boost::filesystem::last_write_time(it->path(), entry_ec); if (entry_ec || now - written < stale_age) continue; + const std::string name = it->path().filename().string(); + if (name.size() > 4 && name.compare(name.size() - 4, 4, ".old") == 0) { + // A target rename_file() moved aside and never put back or removed: + // the last copy of the file if the target is missing, so it goes back. + const boost::filesystem::path original = it->path().parent_path() / name.substr(0, name.rfind('.', name.size() - 5)); + if (! boost::filesystem::exists(original, entry_ec)) { + boost::filesystem::rename(it->path(), original, entry_ec); + BOOST_LOG_TRIVIAL(warning) << "Restored " << original << " from " << it->path() << (entry_ec ? ": failed" : ""); + continue; + } + } if (boost::filesystem::remove(it->path(), entry_ec)) { BOOST_LOG_TRIVIAL(info) << "Removed stale temporary file " << it->path(); ++ removed; diff --git a/tests/libslic3r/test_utils.cpp b/tests/libslic3r/test_utils.cpp index 77b2bffe2b..7c1fddf823 100644 --- a/tests/libslic3r/test_utils.cpp +++ b/tests/libslic3r/test_utils.cpp @@ -187,9 +187,23 @@ TEST_CASE("write_file_atomically survives two threads writing one target", "[uti REQUIRE(temporaries == 0); } +TEST_CASE("remove_stale_temp_files puts back a moved-aside file whose original is missing", "[utils]") { + ScopedTemporaryDir dir; + REQUIRE_FALSE(write_file_atomically((dir.path() / "lost.json.4242.old").string(), "last copy")); + boost::filesystem::last_write_time(dir.path() / "lost.json.4242.old", std::time(nullptr) - 7200); + + REQUIRE(remove_stale_temp_files(dir.path()) == 0); + REQUIRE(boost::filesystem::exists(dir.path() / "lost.json")); + REQUIRE_FALSE(boost::filesystem::exists(dir.path() / "lost.json.4242.old")); + std::string content; + load_string_file(dir.path() / "lost.json", content); + REQUIRE(content == "last copy"); +} + TEST_CASE("remove_stale_temp_files removes only old ...tmp files", "[utils]") { ScopedTemporaryDir dir; - for (const char *name : { "a.json.123.7.tmp", "b.info.4.0.tmp", "c.json", "d.tmp", "e.json.x.1.tmp", "f.json..tmp", "a.json.99", "a.json.12.tmp", "a.json.123.old", "h.json.old" }) { + // a.json exists, so its .old is a leftover; a moved-aside file without its original is a different case. + for (const char *name : { "a.json", "a.json.123.7.tmp", "b.info.4.0.tmp", "c.json", "d.tmp", "e.json.x.1.tmp", "f.json..tmp", "a.json.99", "a.json.12.tmp", "a.json.123.old", "h.json.old" }) { REQUIRE_FALSE(write_file_atomically((dir.path() / name).string(), "x")); // An hour old: long past the age below which a temporary may still be in flight. boost::filesystem::last_write_time(dir.path() / name, std::time(nullptr) - 3600); @@ -212,7 +226,8 @@ TEST_CASE("remove_stale_temp_files removes only old ...tmp files", (void) entry; ++entries; } - REQUIRE(entries == 8); + REQUIRE(entries == 9); + REQUIRE(boost::filesystem::exists(dir.path() / "a.json")); REQUIRE(boost::filesystem::exists(dir.path() / "h.json.old")); REQUIRE(boost::filesystem::exists(dir.path() / "a.json.99")); REQUIRE(boost::filesystem::exists(dir.path() / "g.json.7.2.tmp"));