From 5181a7fe2634ac912171b96c77390adc12e9ca15 Mon Sep 17 00:00:00 2001 From: Hanif Koh Date: Thu, 24 Sep 2026 02:36:05 +0800 Subject: [PATCH] Lock Config and Preset Files Across Instances and Write Them Atomically Every running instance shares one OrcaSlicer.conf and one user preset tree, and nothing kept their writers apart. Two instances saving at the same moment, or the cloud preset sync thread writing while the GUI thread saved, could interleave, and a reader in another instance could open a preset JSON or .info file between truncate and close and get a partial file, dropping that preset for the session with a parse error. Add InstanceLock, a scoped guard that serialises the threads of one process through a recursive mutex and other processes through an advisory OS file lock, which the OS releases when its holder dies. It is best effort: when the lock file cannot be created, or another instance still holds it after two seconds, the guard logs a warning and lets the write proceed rather than letting a hung instance block every other one. AppConfig::save() and load() hold OrcaSlicer.conf.lock; load is included because the Windows path restores from the .bak copy, and because Windows cannot replace a file another process has open, so an unlocked reader there made the other instance's save fail. Every user preset writer and reader holds user.lock: Preset::save(), save_info(), load_info() and remove_files(), the whole directory scan in load_presets(), the bundle metadata file, the .info removal after a cloud-confirmed delete and the bundle folder removal on unsubscribe. The guard sits at the leaf writers on purpose: set_sync_info_and_save() calls save_info() under the preset collection mutex, so a batch lock around save_user_presets(), which takes that mutex through delete_preset(), would invert the order. Write preset JSON, .info and bundle metadata files through a temporary file beside the target that is renamed over it, so a reader that never waits still sees a complete old or new file. On Linux and macOS rename_file() deleted the target before renaming, leaving a window in which the file did not exist at all, and returned a meaningless error code on failure; rename() replaces atomically, so call it directly. AppConfig::save() now logs a failed final rename instead of dropping the save silently. --- src/libslic3r/AppConfig.cpp | 20 +++- src/libslic3r/AppConfig.hpp | 2 + src/libslic3r/CMakeLists.txt | 2 + src/libslic3r/Config.cpp | 10 +- src/libslic3r/InstanceLock.cpp | 101 ++++++++++++++++++ src/libslic3r/InstanceLock.hpp | 42 ++++++++ src/libslic3r/Preset.cpp | 21 +++- src/libslic3r/Preset.hpp | 4 + src/libslic3r/PresetBundle.cpp | 12 ++- src/libslic3r/Utils.hpp | 4 + src/libslic3r/utils.cpp | 24 ++++- src/slic3r/GUI/GUI_App.cpp | 7 +- tests/libslic3r/CMakeLists.txt | 1 + tests/libslic3r/test_instance_lock.cpp | 135 +++++++++++++++++++++++++ tests/libslic3r/test_utils.cpp | 26 +++++ 15 files changed, 394 insertions(+), 17 deletions(-) create mode 100644 src/libslic3r/InstanceLock.cpp create mode 100644 src/libslic3r/InstanceLock.hpp create mode 100644 tests/libslic3r/test_instance_lock.cpp diff --git a/src/libslic3r/AppConfig.cpp b/src/libslic3r/AppConfig.cpp index c8a2c4984d..09eacac87d 100644 --- a/src/libslic3r/AppConfig.cpp +++ b/src/libslic3r/AppConfig.cpp @@ -5,6 +5,7 @@ //BBS #include "Preset.hpp" #include "Exception.hpp" +#include "InstanceLock.hpp" #include "LocalesUtils.hpp" #include "Thread.hpp" #include "format.hpp" @@ -734,6 +735,9 @@ std::string AppConfig::load() { json j; + // Keep another instance from replacing or restoring the file mid-read. + InstanceLock instance_lock(lock_path()); + // 1) Read the complete config file into a boost::property_tree. namespace pt = boost::property_tree; pt::ptree tree; @@ -983,6 +987,7 @@ void AppConfig::save() // The config is first written to a file with a PID suffix and then moved // to avoid race conditions with multiple instances of Slic3r const auto path = config_path(); + InstanceLock instance_lock(lock_path()); std::string path_pid = (boost::format("%1%.%2%") % path % get_current_pid()).str(); json j; @@ -1142,7 +1147,8 @@ void AppConfig::save() // Rename the config atomically. // On Windows, the rename is likely NOT atomic, thus it may fail if PrusaSlicer crashes on another thread in the meanwhile. // To cope with that, we already made a backup of the config on Windows. - rename_file(path_pid, path); + if (const std::error_code ec = rename_file(path_pid, path)) + BOOST_LOG_TRIVIAL(error) << "Failed to replace " << path << " with the new configuration: " << ec.message(); m_dirty = false; } @@ -1150,6 +1156,9 @@ void AppConfig::save() std::string AppConfig::load() { + // Keep another instance from replacing or restoring the file mid-read. + InstanceLock instance_lock(lock_path()); + // 1) Read the complete config file into a boost::property_tree. namespace pt = boost::property_tree; pt::ptree tree; @@ -1287,6 +1296,7 @@ void AppConfig::save() // The config is first written to a file with a PID suffix and then moved // to avoid race conditions with multiple instances of Slic3r const auto path = config_path(); + InstanceLock instance_lock(lock_path()); std::string path_pid = (boost::format("%1%.%2%") % path % get_current_pid()).str(); std::stringstream config_ss; @@ -1351,7 +1361,8 @@ void AppConfig::save() // Rename the config atomically. // On Windows, the rename is likely NOT atomic, thus it may fail if PrusaSlicer crashes on another thread in the meanwhile. // To cope with that, we already made a backup of the config on Windows. - rename_file(path_pid, path); + if (const std::error_code ec = rename_file(path_pid, path)) + BOOST_LOG_TRIVIAL(error) << "Failed to replace " << path << " with the new configuration: " << ec.message(); m_dirty = false; } #endif @@ -1844,6 +1855,11 @@ void AppConfig::reset_selections() } } +std::string AppConfig::lock_path() +{ + return Slic3r::data_dir().empty() ? std::string() : config_path() + ".lock"; +} + std::string AppConfig::config_path() { #ifdef USE_JSON_CONFIG diff --git a/src/libslic3r/AppConfig.hpp b/src/libslic3r/AppConfig.hpp index 24a4c2069b..4c71aa9c03 100644 --- a/src/libslic3r/AppConfig.hpp +++ b/src/libslic3r/AppConfig.hpp @@ -338,6 +338,8 @@ public: // Get the default config path from Slic3r::data_dir(). std::string config_path(); + // Lock file guarding config_path() against other running instances; empty without a data dir. + std::string lock_path(); // Returns true if the user's data directory comes from before Slic3r 1.40.0 (no updating) bool legacy_datadir() const { return m_legacy_datadir; } diff --git a/src/libslic3r/CMakeLists.txt b/src/libslic3r/CMakeLists.txt index e734c036fa..6d5f850239 100644 --- a/src/libslic3r/CMakeLists.txt +++ b/src/libslic3r/CMakeLists.txt @@ -304,6 +304,8 @@ set(lisbslic3r_sources Geometry/VoronoiUtils.cpp Geometry/VoronoiUtils.hpp Geometry/VoronoiVisualUtils.hpp + InstanceLock.cpp + InstanceLock.hpp Int128.hpp KDTreeIndirect.hpp Layer.cpp diff --git a/src/libslic3r/Config.cpp b/src/libslic3r/Config.cpp index c8d816b3a4..177cd63c66 100644 --- a/src/libslic3r/Config.cpp +++ b/src/libslic3r/Config.cpp @@ -1522,12 +1522,10 @@ void ConfigBase::save_to_json(const std::string &file, const std::string &name, // Serialize first: if that throws (invalid UTF-8), the existing file stays untouched. std::ostringstream ss; this->save_to_json(ss, name, from, version); - boost::nowide::ofstream c; - c.open(file, std::ios::out | std::ios::trunc); - c << ss.str(); - c.close(); - - BOOST_LOG_TRIVIAL(info) << __FUNCTION__ << ":" <<__LINE__ << boost::format(", saved config to %1%\n")%file; + if (const std::error_code ec = write_file_atomically(file, ss.str())) + BOOST_LOG_TRIVIAL(error) << __FUNCTION__ << boost::format(": failed to save config to %1%: %2%") % file % ec.message(); + else + BOOST_LOG_TRIVIAL(info) << __FUNCTION__ << ":" <<__LINE__ << boost::format(", saved config to %1%\n")%file; } void ConfigBase::save_to_json(std::ostream &os, const std::string &name, const std::string &from, const std::string &version, bool replace_invalid_utf8) const diff --git a/src/libslic3r/InstanceLock.cpp b/src/libslic3r/InstanceLock.cpp new file mode 100644 index 0000000000..f0d55077dc --- /dev/null +++ b/src/libslic3r/InstanceLock.cpp @@ -0,0 +1,101 @@ +#include "InstanceLock.hpp" + +#include +#include +#include +#include + +#include +#include +#include +#ifdef _WIN32 +#include +#endif + +namespace Slic3r { + +// One slot per lock file, shared by every guard in the process. A POSIX fcntl +// lock is owned by the process, not the thread or descriptor, so a single +// file_lock 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. +struct InstanceLock::Slot +{ + std::recursive_mutex mutex; + // Absent when the lock file could not be created or opened. + std::unique_ptr file_lock; + int depth{0}; + bool file_locked{false}; +}; + +InstanceLock::Slot &InstanceLock::slot_for(const std::string &lock_file_path) +{ + static std::mutex registry_mutex; + // Never freed: the slots keep the lock files open for as long as anything + // in the process may still save, including during static destruction. + static auto *registry = new std::map>(); + + std::lock_guard guard(registry_mutex); + std::unique_ptr &slot = (*registry)[lock_file_path]; + if (! slot) { + slot = std::make_unique(); + try { + // file_lock only opens existing files. + boost::nowide::ofstream(lock_file_path, std::ios::app).close(); +#ifdef _WIN32 + slot->file_lock = std::make_unique(boost::nowide::widen(lock_file_path).c_str()); +#else + slot->file_lock = std::make_unique(lock_file_path.c_str()); +#endif + } catch (const std::exception &e) { + BOOST_LOG_TRIVIAL(warning) << "Cannot open lock file " << lock_file_path << ": " << e.what() + << "; other instances are not excluded from writing"; + } + } + return *slot; +} + +InstanceLock::InstanceLock(const std::string &lock_file_path, std::chrono::milliseconds timeout) +{ + if (lock_file_path.empty()) + return; + 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; + for (;;) { + try { + if (m_slot->file_lock->try_lock()) { + m_slot->file_locked = true; + break; + } + } catch (const std::exception &e) { + BOOST_LOG_TRIVIAL(warning) << "Cannot lock " << lock_file_path << ": " << e.what(); + break; + } + if (std::chrono::steady_clock::now() >= deadline) { + BOOST_LOG_TRIVIAL(warning) << "Another instance has held " << lock_file_path << " for over " + << timeout.count() << " ms; writing without the lock"; + break; + } + std::this_thread::sleep_for(std::chrono::milliseconds(5)); + } + } + m_locked = m_slot->file_locked; +} + +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(); + } + m_slot->file_locked = false; + } + m_slot->mutex.unlock(); +} + +} // namespace Slic3r diff --git a/src/libslic3r/InstanceLock.hpp b/src/libslic3r/InstanceLock.hpp new file mode 100644 index 0000000000..936cafdac9 --- /dev/null +++ b/src/libslic3r/InstanceLock.hpp @@ -0,0 +1,42 @@ +#pragma once + +#include +#include + +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. +// +// 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. +class InstanceLock +{ +public: + static constexpr std::chrono::milliseconds default_timeout{2000}; + + // An empty path makes the guard a no-op. + explicit InstanceLock(const std::string &lock_file_path, std::chrono::milliseconds timeout = default_timeout); + ~InstanceLock(); + + InstanceLock(const InstanceLock &) = delete; + InstanceLock &operator=(const InstanceLock &) = delete; + + // True while this process holds the cross-process file lock. + bool locked() const { return m_locked; } + +private: + struct Slot; + static Slot &slot_for(const std::string &lock_file_path); + + Slot *m_slot{nullptr}; + bool m_locked{false}; +}; + +} // namespace Slic3r diff --git a/src/libslic3r/Preset.cpp b/src/libslic3r/Preset.cpp index 1002f5be89..b38c878cdb 100644 --- a/src/libslic3r/Preset.cpp +++ b/src/libslic3r/Preset.cpp @@ -48,6 +48,9 @@ #include "libslic3r.h" #include "Utils.hpp" +#include "InstanceLock.hpp" + +#include #include "Time.hpp" #include "PlaceholderParser.hpp" #include "libslic3r/GCode/Thumbnails.hpp" @@ -104,6 +107,11 @@ std::string get_preset_canonical_name(const std::string &preset_bare_name, const } } +std::string user_presets_lock_path() +{ + return data_dir().empty() ? std::string() : (fs::path(data_dir()) / (PRESET_USER_DIR ".lock")).string(); +} + std::string get_preset_bare_name(const std::string &canonical_name) { const auto pos = canonical_name.find_last_of('/'); @@ -603,6 +611,7 @@ Preset::Type Preset::get_type_from_string(std::string type_str) void Preset::load_info(const std::string& file) { + InstanceLock instance_lock(user_presets_lock_path()); try { boost::property_tree::ptree tree; boost::nowide::ifstream ifs(file); @@ -646,18 +655,20 @@ void Preset::save_info(std::string file) file = idx_file.string(); } - boost::nowide::ofstream c; - c.open(file, std::ios::out | std::ios::trunc); std::string sync_info_to_save; //BBS: hold is used for stop requesting to server this time if (this->sync_info.compare("hold") != 0) sync_info_to_save = this->sync_info; + std::ostringstream c; c << "sync_info" << " = " << sync_info_to_save << std::endl; c << "user_id" << " = " << this->user_id << std::endl; c << "setting_id" << " = " << this->setting_id << std::endl; c << "base_id" << " = " << this->base_id << std::endl; c << "updated_time" << " = " << std::to_string(this->updated_time) << std::endl; - c.close(); + + InstanceLock instance_lock(user_presets_lock_path()); + if (const std::error_code ec = write_file_atomically(file, c.str())) + BOOST_LOG_TRIVIAL(error) << __FUNCTION__ << ": failed to save " << file << ": " << ec.message(); } void Preset::remove_files(bool cloud_already_deleted) @@ -666,6 +677,7 @@ void Preset::remove_files(bool cloud_already_deleted) if (this->is_project_embedded) { return; } + InstanceLock instance_lock(user_presets_lock_path()); // Erase the preset file. boost::nowide::remove(this->file.c_str()); fs::path idx_path(this->file); @@ -702,6 +714,7 @@ void Preset::save(DynamicPrintConfig* parent_config) else from_str = std::string("Default"); + InstanceLock instance_lock(user_presets_lock_path()); boost::filesystem::create_directories(fs::path(this->file).parent_path()); const std::string bare_name = get_preset_bare_name(this->name); @@ -1704,6 +1717,8 @@ 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()); //BBS: change to json format for (auto &dir_entry : boost::filesystem::directory_iterator(dir)) { diff --git a/src/libslic3r/Preset.hpp b/src/libslic3r/Preset.hpp index c66518e29d..aef97021b7 100644 --- a/src/libslic3r/Preset.hpp +++ b/src/libslic3r/Preset.hpp @@ -484,6 +484,10 @@ std::string get_preset_canonical_name(const std::string &preset_bare_name, const // Tail segment of a canonical name — what's written to the bundle's .json filename and JSON "name" field. std::string get_preset_bare_name(const std::string &canonical_name); +// Lock file guarding every user preset file under data_dir() against other +// running instances and the preset sync thread; empty without a data dir. +std::string user_presets_lock_path(); + // 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()); diff --git a/src/libslic3r/PresetBundle.cpp b/src/libslic3r/PresetBundle.cpp index dc731b63c4..013e55fcd6 100644 --- a/src/libslic3r/PresetBundle.cpp +++ b/src/libslic3r/PresetBundle.cpp @@ -12,6 +12,7 @@ #include "libslic3r.h" #include "I18N.hpp" #include "Utils.hpp" +#include "InstanceLock.hpp" #include "LocalesUtils.hpp" #include "Model.hpp" #include "TriangleSelector.hpp" @@ -2231,6 +2232,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); @@ -7867,6 +7869,7 @@ bool PresetBundle::check_duplicate_filament_subtypes() const // Orca: BundleMetadata method implementations bool BundleMetadata::load_from_json(const std::string& path) { + InstanceLock instance_lock(user_presets_lock_path()); try { boost::nowide::ifstream ifs(path); if (!ifs.good()) @@ -7928,13 +7931,16 @@ 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); - boost::nowide::ofstream ofs(path); - ofs << j.dump(4); - return ofs.good(); + if (const std::error_code ec = write_file_atomically(path, j.dump(4))) { + BOOST_LOG_TRIVIAL(error) << "Failed to save bundle metadata to " << path << ": " << ec.message(); + return false; + } + return true; } catch (const std::exception& e) { BOOST_LOG_TRIVIAL(error) << "Failed to save bundle metadata to " << path << ": " << e.what(); return false; diff --git a/src/libslic3r/Utils.hpp b/src/libslic3r/Utils.hpp index b21da72fc8..64950a64f5 100644 --- a/src/libslic3r/Utils.hpp +++ b/src/libslic3r/Utils.hpp @@ -224,6 +224,10 @@ extern std::vector split_string(const std::string &str, char delimi // On Windows, the file explorer (or anti-virus or whatever else) often locks the file // for a short while, so the file may not be movable. Retry while we see recoverable errors. extern std::error_code rename_file(const std::string &from, const std::string &to); +// Write `content` to `path` through a temporary file beside it that is then renamed +// over the target, so a concurrent reader sees the old or the new file, never a +// partial one. The temporary file is removed on failure. +extern std::error_code write_file_atomically(const std::string &path, const std::string &content); enum CopyFileResult { SUCCESS = 0, diff --git a/src/libslic3r/utils.cpp b/src/libslic3r/utils.cpp index 9def5dad17..97c619c517 100644 --- a/src/libslic3r/utils.cpp +++ b/src/libslic3r/utils.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include #include #include @@ -704,11 +705,30 @@ std::error_code rename_file(const std::string &from, const std::string &to) #ifdef _WIN32 return WindowsSupport::rename(from, to); #else - boost::nowide::remove(to.c_str()); - return std::make_error_code(static_cast(boost::nowide::rename(from.c_str(), to.c_str()))); + // rename(2) replaces an existing target atomically; removing it first would + // leave a window in which the file does not exist at all. + return std::make_error_code(static_cast(boost::nowide::rename(from.c_str(), to.c_str()) == 0 ? 0 : errno)); #endif } +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); + } + } + std::error_code ec = rename_file(tmp_path, path); + if (ec) + boost::nowide::remove(tmp_path.c_str()); + return ec; +} + #ifdef __linux__ // Copied from boost::filesystem. // Called by copy_file_linux() in case linux sendfile() API is not supported. diff --git a/src/slic3r/GUI/GUI_App.cpp b/src/slic3r/GUI/GUI_App.cpp index 48bd58b1a5..2df8dca3b4 100644 --- a/src/slic3r/GUI/GUI_App.cpp +++ b/src/slic3r/GUI/GUI_App.cpp @@ -85,6 +85,7 @@ #include "libslic3r/Model.hpp" #include "libslic3r/I18N.hpp" #include "libslic3r/PresetBundle.hpp" +#include "libslic3r/InstanceLock.hpp" #include "libslic3r/Thread.hpp" #include "libslic3r/miniz_extension.hpp" #include "libslic3r/Utils.hpp" @@ -7624,7 +7625,10 @@ 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; - boost::filesystem::remove_all(bundle_folder, ec); + { + InstanceLock instance_lock(user_presets_lock_path()); + boost::filesystem::remove_all(bundle_folder, ec); + } preset_bundle->bundles.WriteLock(); preset_bundle->bundles.m_bundles.erase(bundle.id); @@ -8889,6 +8893,7 @@ void GUI_App::preset_deleted_from_cloud(std::string setting_id) // Delete the .info file after cloud deletion is confirmed if (!preset_file_path.empty() && fs::exists(fs::path(preset_file_path))) { + InstanceLock instance_lock(user_presets_lock_path()); boost::nowide::remove(preset_file_path.c_str()); BOOST_LOG_TRIVIAL(info) << "Deleted .info file after cloud confirmation: " << preset_file_path; } diff --git a/tests/libslic3r/CMakeLists.txt b/tests/libslic3r/CMakeLists.txt index 185dce37da..c064d9e5e3 100644 --- a/tests/libslic3r/CMakeLists.txt +++ b/tests/libslic3r/CMakeLists.txt @@ -49,6 +49,7 @@ add_executable(${_TEST_NAME}_tests test_ordering_strategies.cpp # test_png_io.cpp test_indexed_triangle_set.cpp + test_instance_lock.cpp ../libnest2d/printer_parts.cpp ) diff --git a/tests/libslic3r/test_instance_lock.cpp b/tests/libslic3r/test_instance_lock.cpp new file mode 100644 index 0000000000..e28fa3420c --- /dev/null +++ b/tests/libslic3r/test_instance_lock.cpp @@ -0,0 +1,135 @@ +#include + +#include +#include +#include + +#include + +#include "libslic3r/InstanceLock.hpp" +#include "test_utils.hpp" + +#ifndef _WIN32 +#include +#include +#include +#endif + +using namespace Slic3r; +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(); + + { + InstanceLock lock(path); + REQUIRE(lock.locked()); + REQUIRE(boost::filesystem::exists(path)); + } + // Released: a fresh guard gets the lock at once instead of waiting out a timeout. + const auto started = std::chrono::steady_clock::now(); + InstanceLock again(path, 5000ms); + REQUIRE(again.locked()); + REQUIRE(std::chrono::steady_clock::now() - started < 1000ms); +} + +TEST_CASE("InstanceLock nests within one thread", "[InstanceLock]") +{ + ScopedTemporaryDir dir; + const std::string path = (dir.path() / "shared.lock").string(); + + InstanceLock outer(path); + { + InstanceLock inner(path, 100ms); + REQUIRE(inner.locked()); + } + // The inner guard leaving does not release the outer one. + REQUIRE(outer.locked()); +} + +TEST_CASE("InstanceLock is a no-op for an empty path and survives an unwritable one", "[InstanceLock]") +{ + ScopedTemporaryDir dir; + + InstanceLock none(""); + REQUIRE_FALSE(none.locked()); + + // The directory does not exist, so the lock file cannot be created; the + // guard still constructs and the write it guards can go ahead. + InstanceLock unwritable((dir.path() / "missing" / "shared.lock").string(), 100ms); + REQUIRE_FALSE(unwritable.locked()); +} + +TEST_CASE("InstanceLock serialises the threads of one process", "[InstanceLock]") +{ + ScopedTemporaryDir dir; + const std::string path = (dir.path() / "shared.lock").string(); + + std::atomic holder_ready{false}; + std::atomic holder_released{false}; + std::thread holder([&] { + InstanceLock lock(path); + holder_ready = true; + std::this_thread::sleep_for(150ms); + holder_released = true; + }); + while (! holder_ready) + std::this_thread::yield(); + + bool released_before_acquire = false; + { + InstanceLock lock(path); + released_before_acquire = holder_released; + } + holder.join(); + REQUIRE(released_before_acquire); +} + +#ifndef _WIN32 +// The cross-process side of the lock is a POSIX fcntl write lock, which a +// child process takes here directly; the same primitive backs the guard on +// 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(); + + int child_holds[2], child_may_exit[2]; + REQUIRE(::pipe(child_holds) == 0); + REQUIRE(::pipe(child_may_exit) == 0); + + const pid_t child = ::fork(); + REQUIRE(child >= 0); + if (child == 0) { + int fd = ::open(path.c_str(), O_RDWR | O_CREAT, 0644); + struct flock lock{}; + lock.l_type = F_WRLCK; + lock.l_whence = SEEK_SET; + char byte = ::fcntl(fd, F_SETLK, &lock) == 0 ? '1' : '0'; + if (::write(child_holds[1], &byte, 1) != 1 || ::read(child_may_exit[0], &byte, 1) != 1) + ::_exit(1); + ::_exit(0); + } + + char byte = '0'; + REQUIRE(::read(child_holds[0], &byte, 1) == 1); + REQUIRE(byte == '1'); + + bool locked_while_child_holds; + { + InstanceLock lock(path, 100ms); + locked_while_child_holds = lock.locked(); + } + REQUIRE(::write(child_may_exit[1], "x", 1) == 1); + int status = 0; + REQUIRE(::waitpid(child, &status, 0) == child); + for (int fd : {child_holds[0], child_holds[1], child_may_exit[0], child_may_exit[1]}) + ::close(fd); + + REQUIRE_FALSE(locked_while_child_holds); + InstanceLock lock(path); + REQUIRE(lock.locked()); +} +#endif diff --git a/tests/libslic3r/test_utils.cpp b/tests/libslic3r/test_utils.cpp index 7880b783f1..36065dbd50 100644 --- a/tests/libslic3r/test_utils.cpp +++ b/tests/libslic3r/test_utils.cpp @@ -62,6 +62,32 @@ TEST_CASE("per-user temp root is unchanged on Windows, isolated elsewhere", "[ut #endif } +TEST_CASE("write_file_atomically replaces the target and leaves no temporary file", "[utils]") { + ScopedTemporaryDir dir; + const boost::filesystem::path target = dir.path() / "preset.json"; + + REQUIRE_FALSE(write_file_atomically(target.string(), "first")); + REQUIRE_FALSE(write_file_atomically(target.string(), "second")); + + std::string content; + load_string_file(target, content); + REQUIRE(content == "second"); + size_t entries = 0; + for (auto &entry : boost::filesystem::directory_iterator(dir.path())) { + (void) entry; + ++entries; + } + REQUIRE(entries == 1); +} + +TEST_CASE("write_file_atomically reports a missing directory and writes nothing", "[utils]") { + ScopedTemporaryDir dir; + const boost::filesystem::path target = dir.path() / "missing" / "preset.json"; + + REQUIRE(write_file_atomically(target.string(), "x")); + REQUIRE_FALSE(boost::filesystem::exists(target)); +} + TEST_CASE("copy_file reports the OS error when the destination cannot be written", "[utils]") { ScopedTemporaryFile source(".txt"); {