From 758b802dec53317e3192252c00d4f36bcc76860e Mon Sep 17 00:00:00 2001 From: Hanif Koh Date: Thu, 24 Sep 2026 20:54:02 +0800 Subject: [PATCH] Lock the Preset Scan Per File and Reopen a Lock File Replaced on Disk Holding the lock across the whole preset scan meant a reload on a background thread, which the login path runs, blocked a save on the GUI thread for the scan's duration through the in-process mutex, which has no timeout. Each file is locked on its own now, which keeps a file whole under a reader without keeping the saver waiting. A guard kept its handle to the lock file for good, so a lock file that someone deleted or recreated left this instance locking a file no other instance could see. The guard compares what the path names against what it opened and reopens when they differ. Preset::save() serialises before it takes the lock, so the exclusive window is the two file writes. On Windows an unlocked reader, which the CLI and a timed-out instance are by design, made the rename fail at once and the write go in place under that reader; the rename is retried for half a second first, since a reader is done in milliseconds, and the fallback when no temporary can be created is logged like the other one. The sweep matches only the exact ...tmp shape and waits an hour, since hosts sharing a data dir may disagree on the time. Real write access is checked with access(), the read-only tests skip as root, and the dead permissions block after the rename is gone. --- src/libslic3r/InstanceLock.cpp | 12 +++++ src/libslic3r/Preset.cpp | 62 ++++++++++++--------- src/libslic3r/Utils.hpp | 5 +- src/libslic3r/utils.cpp | 74 +++++++++++++++++++------- tests/libslic3r/test_instance_lock.cpp | 18 +++++++ tests/libslic3r/test_utils.cpp | 26 +++++---- tests/libslic3r/test_vendor_cache.cpp | 8 +++ 7 files changed, 149 insertions(+), 56 deletions(-) diff --git a/src/libslic3r/InstanceLock.cpp b/src/libslic3r/InstanceLock.cpp index 13d6cc950e..2cfe7ac8f9 100644 --- a/src/libslic3r/InstanceLock.cpp +++ b/src/libslic3r/InstanceLock.cpp @@ -11,6 +11,8 @@ #include #endif +#include "Utils.hpp" + namespace Slic3r { // One slot per lock file, shared by every guard in the process. A POSIX fcntl @@ -22,6 +24,9 @@ struct InstanceLock::Slot std::recursive_mutex mutex; // Absent until the lock file has been created and opened. 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. + std::string identity; int depth{0}; bool file_locked{false}; // After a timed-out wait, guards skip waiting until this point. @@ -57,6 +62,7 @@ bool InstanceLock::open_lock_file(Slot &slot, const std::string &lock_file_path) #else slot.file_lock = std::make_unique(lock_file_path.c_str()); #endif + slot.identity = file_identity(lock_file_path); return true; } catch (const std::exception &e) { slot.retry_at = std::chrono::steady_clock::now() + cooldown; @@ -73,6 +79,12 @@ InstanceLock::InstanceLock(const std::string &lock_file_path, std::chrono::milli m_slot = &slot_for(lock_file_path); m_slot_guard = std::unique_lock(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 && 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))) { const bool wait = now >= m_slot->skip_waiting_until; diff --git a/src/libslic3r/Preset.cpp b/src/libslic3r/Preset.cpp index 23084eecd6..d96aa4a766 100644 --- a/src/libslic3r/Preset.cpp +++ b/src/libslic3r/Preset.cpp @@ -765,10 +765,15 @@ void Preset::save(DynamicPrintConfig* parent_config) to_save = &temp_config; } + std::ostringstream json; + to_save->save_to_json(json, bare_name, from_str, this->version.to_string()); + InstanceLock instance_lock(user_presets_lock_path()); boost::filesystem::create_directories(fs::path(this->file).parent_path()); - to_save->save_to_json(this->file, bare_name, from_str, this->version.to_string()); - BOOST_LOG_TRIVIAL(info) << __FUNCTION__ << " save config for: " << this->name << " and filament_id: " << filament_id << " and base_id: " << this->base_id; + if (const std::error_code ec = write_file_atomically(this->file, json.str())) + BOOST_LOG_TRIVIAL(error) << __FUNCTION__ << ": failed to save " << this->file << ": " << ec.message(); + else + BOOST_LOG_TRIVIAL(info) << __FUNCTION__ << " save config for: " << this->name << " and filament_id: " << filament_id << " and base_id: " << this->base_id; // Bundle presets are synced via bundle_id and don't need individual .info files. if (! this->is_from_bundle()) { @@ -1722,10 +1727,11 @@ void PresetCollection::load_presets( std::set *key_set1 = nullptr, *key_set2 = nullptr; Preset::get_extruder_names_and_keysets(m_type, extruder_id_name, extruder_variant_name, &key_set1, &key_set2); - // Held across the scan so no instance replaces or removes a file mid-read. - InstanceLock instance_lock(user_presets_lock_path(read_only)); - if (instance_lock.locked()) - remove_stale_temp_files(dir); + { + InstanceLock instance_lock(user_presets_lock_path(read_only)); + if (instance_lock.locked()) + remove_stale_temp_files(dir); + } //BBS: change to json format for (auto &dir_entry : boost::filesystem::directory_iterator(dir)) { @@ -1747,30 +1753,36 @@ void PresetCollection::load_presets( preset.file = dir_entry.path().string(); // Load the preset file, apply preset values on top of defaults. try { - fs::path idx_path(preset.file); - idx_path.replace_extension(".info"); - if (fs::exists(idx_path)) { - preset.load_info(idx_path.string()); - } DynamicPrintConfig config; - //BBS: change to json format - //ConfigSubstitutions config_substitutions = config.load_from_ini(preset.file, substitution_rule); std::map key_values; std::string reason; - ConfigSubstitutions config_substitutions = config.load_from_json(preset.file, substitution_rule, key_values, reason); + ConfigSubstitutions config_substitutions; + { + // Per file, so no instance replaces or removes it mid-read, and a + // save on another thread never waits for the whole scan. + InstanceLock instance_lock(user_presets_lock_path(read_only)); + fs::path idx_path(preset.file); + idx_path.replace_extension(".info"); + if (fs::exists(idx_path)) { + preset.load_info(idx_path.string()); + } + //BBS: change to json format + //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); + BOOST_LOG_TRIVIAL(error) << boost::format("parse config %1% failed")%preset.file; + ++m_errors; + continue; + } + } if (! config_substitutions.empty()) substitutions.push_back({ preset.name, m_type, PresetConfigSubstitutions::Source::UserFile, preset.file, std::move(config_substitutions) }); - 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); - BOOST_LOG_TRIVIAL(error) << boost::format("parse config %1% failed")%preset.file; - ++m_errors; - continue; - } std::string version_str = key_values[BBL_JSON_KEY_VERSION]; boost::optional version = Semver::parse(version_str); diff --git a/src/libslic3r/Utils.hpp b/src/libslic3r/Utils.hpp index 9de742dc14..380190ac64 100644 --- a/src/libslic3r/Utils.hpp +++ b/src/libslic3r/Utils.hpp @@ -235,9 +235,12 @@ extern std::error_code write_file_atomically(const std::string &path, std::initi inline std::error_code write_file_atomically(const std::string &path, const std::string &content, bool binary = false) { return write_file_atomically(path, { std::string_view(content) }, binary); } // Remove the `...tmp` files a crashed write_file_atomically() left in -// `dir` at least ten minutes ago, only names starting with `name_prefix` when it is +// `dir` at least an hour ago, only names starting with `name_prefix` when it is // given. Meant for directories the application owns. 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, diff --git a/src/libslic3r/utils.cpp b/src/libslic3r/utils.cpp index 5f3cf6680c..baf04117a9 100644 --- a/src/libslic3r/utils.cpp +++ b/src/libslic3r/utils.cpp @@ -10,6 +10,8 @@ #include #include #include +#include +#include #include #include #include @@ -767,50 +769,84 @@ std::error_code write_file_atomically(const std::string &path, std::initializer_ if (std::error_code ec = write_whole_file(tmp_path, chunks, binary)) { boost::nowide::remove(tmp_path.c_str()); // A directory that allows writing the file but not creating one beside it. - if (target_exists && ec == std::errc::permission_denied) + if (target_exists && ec == std::errc::permission_denied) { + BOOST_LOG_TRIVIAL(warning) << "Cannot create a temporary beside " << path << " (" << ec.message() << "); writing in place"; return write_whole_file(path, chunks, binary); + } return ec; } #ifndef _WIN32 - // On Windows a read-only bit on the temporary would stop the rename itself. + // Only here: on Windows the target was refused above unless writable, and + // a read-only bit on the temporary would stop the rename itself. if (target_exists) boost::filesystem::permissions(tmp_path, target.permissions(), bec); #endif - std::error_code ec = rename_file(tmp_path, path); + // Windows refuses to replace a file another process holds open without + // FILE_SHARE_DELETE, which is how the C runtime opens files for reading; a + // reader is done in milliseconds, so wait it out before giving up on the + // atomic path. + std::error_code ec; + for (int attempt = 0; attempt < 20; ++ attempt) { + ec = rename_file(tmp_path, path); + if (ec != std::errc::permission_denied) + break; + std::this_thread::sleep_for(std::chrono::milliseconds(25)); + } if (ec) { boost::nowide::remove(tmp_path.c_str()); - // Windows refuses to replace a file another process holds open without - // FILE_SHARE_DELETE, which is how the C runtime opens files for reading, - // and some mounts cannot replace a file in one step 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. + // Still refused, or a mount that cannot replace a file in one step 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"; return write_whole_file(path, chunks, binary); } -#ifdef _WIN32 - if (target_exists) - boost::filesystem::permissions(path, target.permissions(), bec); -#endif 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 std::to_string(info.dwVolumeSerialNumber) + ":" + std::to_string((static_cast(info.nFileIndexHigh) << 32) | info.nFileIndexLow); +#else + struct stat st; + if (::stat(path.c_str(), &st) != 0) + return {}; + return std::to_string(static_cast(st.st_dev)) + ":" + std::to_string(static_cast(st.st_ino)); +#endif +} + size_t remove_stale_temp_files(const boost::filesystem::path &dir, const std::string &name_prefix) { - // ....tmp + // .....tmp, exactly the shape write_file_atomically() makes. auto is_temp_name = [&name_prefix](const std::string &name) { if (name.compare(0, name_prefix.size(), name_prefix) != 0) return false; static const std::string suffix = ".tmp"; if (name.size() <= suffix.size() || name.compare(name.size() - suffix.size(), suffix.size(), suffix) != 0) return false; - const size_t digits_end = name.size() - suffix.size(); - const size_t dot = name.rfind('.', digits_end - 1); - return dot != std::string::npos && dot != 0 && dot + 1 != digits_end && - std::all_of(name.begin() + dot + 1, name.begin() + digits_end, [](char c) { return c >= '0' && c <= '9'; }); + size_t end = name.size() - suffix.size(); + for (int segment = 0; segment < 2; ++ segment) { + const size_t dot = name.rfind('.', end - 1); + if (dot == std::string::npos || dot == 0 || dot + 1 == end || + ! std::all_of(name.begin() + dot + 1, name.begin() + end, [](char c) { return c >= '0' && c <= '9'; })) + return false; + end = dot; + } + return true; }; // An instance that gave up waiting for the lock writes unlocked by design, - // so a temporary this young may still be in flight; a crash leftover is old. - constexpr std::time_t stale_age = 10 * 60; + // so a temporary this young may still be in flight, and hosts sharing a + // data dir may disagree on the time by minutes; a crash leftover is old. + constexpr std::time_t stale_age = 60 * 60; const std::time_t now = std::time(nullptr); size_t removed = 0; boost::system::error_code ec; diff --git a/tests/libslic3r/test_instance_lock.cpp b/tests/libslic3r/test_instance_lock.cpp index 6c7ac43fb8..6ff9cca250 100644 --- a/tests/libslic3r/test_instance_lock.cpp +++ b/tests/libslic3r/test_instance_lock.cpp @@ -96,6 +96,24 @@ TEST_CASE("InstanceLock retries a lock file it could not open once the cool-down REQUIRE(after_cooldown); } +TEST_CASE("InstanceLock reopens a lock file that was replaced on disk", "[InstanceLock]") +{ + ScopedTemporaryFile lock_file(".lock"); + const std::string path = lock_file.string(); + { + InstanceLock lock(path); + REQUIRE(lock.locked()); + } + + 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.) + REQUIRE(boost::filesystem::exists(path)); +} + TEST_CASE("InstanceLock serialises the threads of one process", "[InstanceLock]") { ScopedTemporaryFile lock_file(".lock"); diff --git a/tests/libslic3r/test_utils.cpp b/tests/libslic3r/test_utils.cpp index df10b2985b..23c42a944e 100644 --- a/tests/libslic3r/test_utils.cpp +++ b/tests/libslic3r/test_utils.cpp @@ -93,6 +93,10 @@ TEST_CASE("write_file_atomically reports a missing directory and writes nothing" } TEST_CASE("write_file_atomically refuses a read-only target and leaves it untouched", "[Utils]") { +#ifndef _WIN32 + if (::geteuid() == 0) + SKIP("a read-only file does not stop root"); +#endif ScopedTemporaryDir dir; const boost::filesystem::path target = dir.path() / "pinned.json"; REQUIRE_FALSE(write_file_atomically(target.string(), "pinned")); @@ -164,33 +168,33 @@ TEST_CASE("write_file_atomically survives two threads writing one target", "[Uti REQUIRE(temporaries == 0); } -TEST_CASE("remove_stale_temp_files removes only old ..tmp files", "[Utils]") { +TEST_CASE("remove_stale_temp_files removes only old ...tmp files", "[Utils]") { ScopedTemporaryDir dir; - for (const char *name : { "a.json.123.tmp", "b.info.4.tmp", "c.json", "d.tmp", "e.json.x.tmp", "f.json..tmp", "a.json.99", "a.json.12.3.tmp" }) { + 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" }) { 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); } // Just written: possibly another instance's in-flight save, so it stays. - REQUIRE_FALSE(write_file_atomically((dir.path() / "g.json.7.tmp").string(), "x")); + REQUIRE_FALSE(write_file_atomically((dir.path() / "g.json.7.2.tmp").string(), "x")); - SECTION("with a name prefix only matching names go, and a numbered backup is not a temporary") { - REQUIRE(remove_stale_temp_files(dir.path(), "a.json") == 2); - REQUIRE_FALSE(boost::filesystem::exists(dir.path() / "a.json.123.tmp")); - REQUIRE_FALSE(boost::filesystem::exists(dir.path() / "a.json.12.3.tmp")); + SECTION("with a name prefix only matching names go; a numbered backup or a one-segment name is not a temporary") { + REQUIRE(remove_stale_temp_files(dir.path(), "a.json") == 1); + REQUIRE_FALSE(boost::filesystem::exists(dir.path() / "a.json.123.7.tmp")); REQUIRE(boost::filesystem::exists(dir.path() / "a.json.99")); - REQUIRE(boost::filesystem::exists(dir.path() / "b.info.4.tmp")); + REQUIRE(boost::filesystem::exists(dir.path() / "a.json.12.tmp")); + REQUIRE(boost::filesystem::exists(dir.path() / "b.info.4.0.tmp")); } SECTION("without a prefix every stale temporary goes and nothing else") { - REQUIRE(remove_stale_temp_files(dir.path()) == 3); + REQUIRE(remove_stale_temp_files(dir.path()) == 2); size_t entries = 0; for (auto &entry : boost::filesystem::directory_iterator(dir.path())) { (void) entry; ++entries; } - REQUIRE(entries == 6); + REQUIRE(entries == 7); REQUIRE(boost::filesystem::exists(dir.path() / "a.json.99")); - REQUIRE(boost::filesystem::exists(dir.path() / "g.json.7.tmp")); + REQUIRE(boost::filesystem::exists(dir.path() / "g.json.7.2.tmp")); } } diff --git a/tests/libslic3r/test_vendor_cache.cpp b/tests/libslic3r/test_vendor_cache.cpp index c290a38285..036c85b7e2 100644 --- a/tests/libslic3r/test_vendor_cache.cpp +++ b/tests/libslic3r/test_vendor_cache.cpp @@ -1,5 +1,9 @@ #include +#ifndef _WIN32 +#include +#endif + #include #include #include @@ -1358,6 +1362,10 @@ TEST_CASE("a header claiming more body than the file holds is rejected", "[Vendo TEST_CASE("a failed write leaves the previous cache in place", "[VendorCache]") { +#ifndef _WIN32 + if (::geteuid() == 0) + SKIP("a read-only file does not stop root"); +#endif TempDir tmp; const std::string cache = (tmp.path / "Durable.opc").string(); REQUIRE(save_one_vendor(cache, one_vendor("Durable"), "Durable", "1.0.0"));