From 651e48723dfe719b9b80c49904df2cd994334603 Mon Sep 17 00:00:00 2001 From: Hanif Koh Date: Thu, 24 Sep 2026 16:26:43 +0800 Subject: [PATCH] Keep a Save From Being Lost to an Open Reader and Cap the Lock Wait Windows refuses to replace a file that another process holds open without FILE_SHARE_DELETE, which is how the C runtime opens files for reading, so the atomic write could fail against a concurrent reader and drop the save where the old in-place write had succeeded. Lock the readers that were still outside the guard, Preset::reload() and the physical printer loader and writer, and when the rename still fails that way, log it and write in place as before; losing the save is worse than a torn read. Report the OS error from a failed write instead of a generic I/O error. Each leaf guard paid the full two-second wait on its own, so a bulk save against an instance holding the lock for a long scan stalled once per preset while holding the preset collection mutex. After a timed-out wait the same lock file is not waited on again for ten seconds. Read-only scans never rewrite or delete and many CLI jobs may share one data dir, so they take no lock and no longer queue behind each other or log about writing. Take the bundle metadata guard beside the write rather than while the JSON is built, state the lock-order rule in the header, and give the tests a temporary file rather than a directory, since the process keeps the lock file open and a directory holding it cannot be removed on Windows. --- src/libslic3r/InstanceLock.cpp | 12 ++++++++-- src/libslic3r/InstanceLock.hpp | 19 +++++++++++---- src/libslic3r/Preset.cpp | 7 +++++- src/libslic3r/PresetBundle.cpp | 5 ++-- src/libslic3r/utils.cpp | 32 +++++++++++++++++++------- tests/libslic3r/test_instance_lock.cpp | 27 +++++++++++++++------- tests/libslic3r/test_utils.cpp | 4 +++- 7 files changed, 80 insertions(+), 26 deletions(-) diff --git a/src/libslic3r/InstanceLock.cpp b/src/libslic3r/InstanceLock.cpp index f0d55077dc..c6bda14f9d 100644 --- a/src/libslic3r/InstanceLock.cpp +++ b/src/libslic3r/InstanceLock.cpp @@ -25,6 +25,8 @@ struct InstanceLock::Slot std::unique_ptr file_lock; int depth{0}; bool file_locked{false}; + // After a timed-out wait, guards skip waiting until this point. + std::chrono::steady_clock::time_point skip_waiting_until{}; }; InstanceLock::Slot &InstanceLock::slot_for(const std::string &lock_file_path) @@ -61,7 +63,9 @@ InstanceLock::InstanceLock(const std::string &lock_file_path, std::chrono::milli m_slot = &slot_for(lock_file_path); m_slot->mutex.lock(); if (m_slot->depth++ == 0 && m_slot->file_lock) { - const auto deadline = std::chrono::steady_clock::now() + timeout; + const auto now = std::chrono::steady_clock::now(); + const bool wait = now >= m_slot->skip_waiting_until; + const auto deadline = now + timeout; for (;;) { try { if (m_slot->file_lock->try_lock()) { @@ -72,9 +76,13 @@ InstanceLock::InstanceLock(const std::string &lock_file_path, std::chrono::milli BOOST_LOG_TRIVIAL(warning) << "Cannot lock " << lock_file_path << ": " << e.what(); break; } + if (! wait) + break; if (std::chrono::steady_clock::now() >= deadline) { + m_slot->skip_waiting_until = deadline + cooldown; BOOST_LOG_TRIVIAL(warning) << "Another instance has held " << lock_file_path << " for over " - << timeout.count() << " ms; writing without the lock"; + << timeout.count() << " ms; proceeding without the lock for the next " + << cooldown.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 936cafdac9..8adc7fc42d 100644 --- a/src/libslic3r/InstanceLock.hpp +++ b/src/libslic3r/InstanceLock.hpp @@ -8,18 +8,29 @@ namespace Slic3r { // Scoped write lock on a file shared by every running instance of the // application, such as the app config or the user preset directory. Threads // of this process are serialised through a recursive mutex, other processes -// through an advisory OS file lock on `lock_file_path`, created on first use. -// The OS releases the file lock when its holder exits, so a crashed instance -// never leaves a stale lock behind. +// through an advisory OS file lock on `lock_file_path`, 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. // // The lock is best effort: when the lock file cannot be created, or another // instance still holds it after `timeout`, 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. +// 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. +// +// 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. +// That is why the guards sit at the leaf readers and writers, and why a +// batch-level guard around save_user_presets(), which takes the collection +// mutex through delete_preset(), must not be added. class InstanceLock { public: static constexpr std::chrono::milliseconds default_timeout{2000}; + static constexpr std::chrono::milliseconds cooldown{10000}; // 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 b38c878cdb..93d141da05 100644 --- a/src/libslic3r/Preset.cpp +++ b/src/libslic3r/Preset.cpp @@ -783,6 +783,7 @@ void Preset::reload(Preset const &parent) std::string reason; ForwardCompatibilitySubstitutionRule substitution_rule = ForwardCompatibilitySubstitutionRule::Disable; try { + InstanceLock instance_lock(user_presets_lock_path()); ConfigSubstitutions config_substitutions = config.load_from_json(file, substitution_rule, key_values, reason); this->config = parent.config; this->config.apply(std::move(config)); @@ -1718,7 +1719,9 @@ void PresetCollection::load_presets( 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()); + // A read-only scan (the CLI) never rewrites or deletes, and many CLI jobs + // may share one data dir, so it does not queue behind the others. + InstanceLock instance_lock(read_only ? std::string() : user_presets_lock_path()); //BBS: change to json format for (auto &dir_entry : boost::filesystem::directory_iterator(dir)) { @@ -4218,6 +4221,7 @@ void PhysicalPrinter::update_preset_names_in_config() void PhysicalPrinter::save(const std::string& file_name_from, const std::string& file_name_to) { + InstanceLock instance_lock(user_presets_lock_path()); // rename the file boost::nowide::rename(file_name_from.data(), file_name_to.data()); this->file = file_name_to; @@ -4334,6 +4338,7 @@ void PhysicalPrinterCollection::load_printers( std::string errors_cummulative; // Store the loaded printers into a new vector, otherwise the binary search for already existing presets would be broken. std::deque printers_loaded; + InstanceLock instance_lock(user_presets_lock_path()); //BBS: change to json format for (auto& dir_entry : boost::filesystem::directory_iterator(dir)) { diff --git a/src/libslic3r/PresetBundle.cpp b/src/libslic3r/PresetBundle.cpp index 013e55fcd6..4dac4d3a49 100644 --- a/src/libslic3r/PresetBundle.cpp +++ b/src/libslic3r/PresetBundle.cpp @@ -7931,12 +7931,13 @@ bool BundleMetadata::save_to_json(const std::string& path) const j["imported_time"] = this->imported_time; j["updated_time"] = this->updated_time; - InstanceLock instance_lock(user_presets_lock_path()); j["print_presets"] = strip_prefix(this->print_presets); j["filament_presets"] = strip_prefix(this->filament_presets); j["printer_presets"] = strip_prefix(this->printer_presets); - if (const std::error_code ec = write_file_atomically(path, j.dump(4))) { + const std::string content = j.dump(4); + InstanceLock instance_lock(user_presets_lock_path()); + if (const std::error_code ec = write_file_atomically(path, content)) { BOOST_LOG_TRIVIAL(error) << "Failed to save bundle metadata to " << path << ": " << ec.message(); return false; } diff --git a/src/libslic3r/utils.cpp b/src/libslic3r/utils.cpp index 97c619c517..654c4962e1 100644 --- a/src/libslic3r/utils.cpp +++ b/src/libslic3r/utils.cpp @@ -711,21 +711,37 @@ std::error_code rename_file(const std::string &from, const std::string &to) #endif } +static std::error_code write_whole_file(const std::string &path, const std::string &content) +{ + errno = 0; + boost::nowide::ofstream out(path, std::ios::out | std::ios::trunc); + out << content; + out.close(); + if (! out.fail()) + return {}; + return std::make_error_code(errno != 0 ? static_cast(errno) : std::errc::io_error); +} + std::error_code write_file_atomically(const std::string &path, const std::string &content) { const std::string tmp_path = path + "." + std::to_string(get_current_pid()) + ".tmp"; - { - boost::nowide::ofstream out(tmp_path, std::ios::out | std::ios::trunc); - out << content; - out.close(); - if (out.fail()) { - boost::nowide::remove(tmp_path.c_str()); - return std::make_error_code(std::errc::io_error); - } + if (std::error_code ec = write_whole_file(tmp_path, content)) { + boost::nowide::remove(tmp_path.c_str()); + return ec; } std::error_code ec = rename_file(tmp_path, path); if (ec) boost::nowide::remove(tmp_path.c_str()); +#ifdef _WIN32 + // Windows refuses to replace a file another process holds open without + // FILE_SHARE_DELETE, which is how the C runtime opens files for reading. + // 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. + if (ec == std::errc::permission_denied) { + BOOST_LOG_TRIVIAL(warning) << "Cannot replace " << path << " while another process holds it open; writing in place"; + ec = write_whole_file(path, content); + } +#endif return ec; } diff --git a/tests/libslic3r/test_instance_lock.cpp b/tests/libslic3r/test_instance_lock.cpp index e28fa3420c..e72d5b8798 100644 --- a/tests/libslic3r/test_instance_lock.cpp +++ b/tests/libslic3r/test_instance_lock.cpp @@ -20,8 +20,8 @@ using namespace std::chrono_literals; TEST_CASE("InstanceLock creates its lock file and holds it for the guard's scope", "[InstanceLock]") { - ScopedTemporaryDir dir; - const std::string path = (dir.path() / "shared.lock").string(); + ScopedTemporaryFile lock_file(".lock"); + const std::string path = lock_file.string(); { InstanceLock lock(path); @@ -37,8 +37,8 @@ TEST_CASE("InstanceLock creates its lock file and holds it for the guard's scope TEST_CASE("InstanceLock nests within one thread", "[InstanceLock]") { - ScopedTemporaryDir dir; - const std::string path = (dir.path() / "shared.lock").string(); + ScopedTemporaryFile lock_file(".lock"); + const std::string path = lock_file.string(); InstanceLock outer(path); { @@ -64,8 +64,8 @@ TEST_CASE("InstanceLock is a no-op for an empty path and survives an unwritable TEST_CASE("InstanceLock serialises the threads of one process", "[InstanceLock]") { - ScopedTemporaryDir dir; - const std::string path = (dir.path() / "shared.lock").string(); + ScopedTemporaryFile lock_file(".lock"); + const std::string path = lock_file.string(); std::atomic holder_ready{false}; std::atomic holder_released{false}; @@ -93,8 +93,8 @@ TEST_CASE("InstanceLock serialises the threads of one process", "[InstanceLock]" // Windows through LockFileEx, but spawning a child there is not worth a test. TEST_CASE("InstanceLock yields to another process and reports it", "[InstanceLock]") { - ScopedTemporaryDir dir; - const std::string path = (dir.path() / "shared.lock").string(); + ScopedTemporaryFile lock_file(".lock"); + const std::string path = lock_file.string(); int child_holds[2], child_may_exit[2]; REQUIRE(::pipe(child_holds) == 0); @@ -122,6 +122,14 @@ TEST_CASE("InstanceLock yields to another process and reports it", "[InstanceLoc InstanceLock lock(path, 100ms); locked_while_child_holds = lock.locked(); } + // The timed-out wait starts a cool-down: the next guard does not wait again. + const auto started = std::chrono::steady_clock::now(); + bool locked_during_cooldown; + { + InstanceLock lock(path, 5000ms); + locked_during_cooldown = lock.locked(); + } + const auto cooldown_wait = std::chrono::steady_clock::now() - started; REQUIRE(::write(child_may_exit[1], "x", 1) == 1); int status = 0; REQUIRE(::waitpid(child, &status, 0) == child); @@ -129,6 +137,9 @@ TEST_CASE("InstanceLock yields to another process and reports it", "[InstanceLoc ::close(fd); REQUIRE_FALSE(locked_while_child_holds); + REQUIRE_FALSE(locked_during_cooldown); + REQUIRE(cooldown_wait < 1000ms); + // A guard inside the cool-down still takes the lock when it is free. InstanceLock lock(path); REQUIRE(lock.locked()); } diff --git a/tests/libslic3r/test_utils.cpp b/tests/libslic3r/test_utils.cpp index 36065dbd50..cac88d30b4 100644 --- a/tests/libslic3r/test_utils.cpp +++ b/tests/libslic3r/test_utils.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #ifndef _WIN32 #include // getuid @@ -84,7 +85,8 @@ TEST_CASE("write_file_atomically reports a missing directory and writes nothing" ScopedTemporaryDir dir; const boost::filesystem::path target = dir.path() / "missing" / "preset.json"; - REQUIRE(write_file_atomically(target.string(), "x")); + const std::error_code ec = write_file_atomically(target.string(), "x"); + REQUIRE(ec == std::errc::no_such_file_or_directory); REQUIRE_FALSE(boost::filesystem::exists(target)); }