From af97d59870646404a99687fa7a4565b9e13686cd Mon Sep 17 00:00:00 2001 From: Hanif Koh Date: Fri, 25 Sep 2026 04:00:33 +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: flock on POSIX, held on the guard's own descriptor so no other close in the process can drop it, and LockFileEx on Windows. The outermost guard opens the lock file and closes it on release, so nothing stays open between saves and a data dir can be removed once nothing is saving into it; the file itself is kept, since deleting it would let a third instance lock a fresh file while the second still holds the old one. It is best effort: when the lock file cannot be opened or locked, or another instance still holds it after a second, the guard logs once and lets the write proceed, then leaves the file alone for ten seconds, so a hung instance never blocks every other one and a holder stuck in a debugger does not cost a stall per save. The guard sits at the leaf readers and writers: set_sync_info_and_save() calls save_info() under the preset collection mutex, so a batch lock around save_user_presets() would invert the order against the sync thread. The preset scan re-takes the guard every 32 files rather than holding it across the scan, and a bundle or user folder is renamed into cache/ under the lock and removed outside it, so a save never waits for a whole tree. Read-only scans, which is what the CLI does, take no lock and create no lock file. AppConfig holds OrcaSlicer.conf.lock in load() and save(); load is included because the Windows path restores from the .bak copy. Every user preset writer and reader holds user.lock: Preset::save(), which writes no .info when the preset itself could not be written, since an .info without its preset reads as a cloud deletion request, save_info(), reload() and remove_files(), each file read by the preset scan, the bundle metadata reads and write, the .info removal after a cloud-confirmed delete, the orphaned-.info scan on the sync thread, the bundle folder removal on unsubscribe and the physical printer writers and delete paths. A bundle import extracts under cache/ into a folder per process and per import, where no scan reads. Preset JSON, .info, bundle metadata, physical printer and config files, and the caches and state files that already used a temporary by hand, now go through write_file_atomically(), which writes ...tmp beside the target and renames it over, so a reader that never waits sees a complete old or new file. A target this process may not write is refused before anything is written, unless the caller says the file was always replaced, as the config was; a symlink is followed; a target that is not a regular file is written in place; and when the rename itself is refused, by a Windows reader holding the file open or a mount that cannot replace in one step, the helper writes in place as before, since losing the save is worse than a torn read. On POSIX the rename replaces the target atomically where the old code removed it first and left a window with no file at all; only a mount that refuses a one-step replace gets the old remove-then-rename. A crash between temporary and rename leaves the temporary, which the scans of the directories the application owns remove once it is an hour old; only the exact shapes this code writes qualify, so a user's numbered backup or an export folder is never touched. A failed config write keeps the config dirty, and the idle handler waits ten seconds before retrying while an explicit save always tries. --- src/libslic3r/AppConfig.cpp | 128 +++++++------ src/libslic3r/AppConfig.hpp | 19 +- src/libslic3r/CMakeLists.txt | 2 + src/libslic3r/Config.cpp | 10 +- src/libslic3r/InstanceLock.cpp | 175 ++++++++++++++++++ src/libslic3r/InstanceLock.hpp | 61 +++++++ src/libslic3r/Preset.cpp | 160 ++++++++++++---- src/libslic3r/Preset.hpp | 14 +- src/libslic3r/PresetBundle.cpp | 42 +++-- src/libslic3r/PresetCacheFormat.cpp | 44 ++--- src/libslic3r/Utils.hpp | 34 ++++ src/libslic3r/utils.cpp | 194 +++++++++++++++++++- src/slic3r/GUI/GUI_App.cpp | 29 +-- src/slic3r/GUI/WebGuideDialog.cpp | 15 +- src/slic3r/Utils/3DPrinterOS.cpp | 8 +- src/slic3r/Utils/OrcaCloudServiceAgent.cpp | 31 +--- src/slic3r/plugin/PluginConfig.cpp | 20 +- tests/libslic3r/CMakeLists.txt | 1 + tests/libslic3r/test_instance_lock.cpp | 202 +++++++++++++++++++++ tests/libslic3r/test_utils.cpp | 189 +++++++++++++++++++ tests/libslic3r/test_vendor_cache.cpp | 19 +- 21 files changed, 1166 insertions(+), 231 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..4e5b0a7139 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" @@ -730,10 +731,15 @@ static bool verify_config_file_checksum(boost::nowide::ifstream &ifs) #ifdef USE_JSON_CONFIG -std::string AppConfig::load() +std::string AppConfig::load(bool read_only) { json j; + // Keep another instance from replacing or restoring the file mid-read. + InstanceLock instance_lock(read_only ? std::string() : lock_path()); + if (instance_lock.locked()) + sweep_leftovers(); + // 1) Read the complete config file into a boost::property_tree. namespace pt = boost::property_tree; pt::ptree tree; @@ -983,7 +989,6 @@ 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(); - std::string path_pid = (boost::format("%1%.%2%") % path % get_current_pid()).str(); json j; @@ -1113,43 +1118,20 @@ void AppConfig::save() j["local_machines"][local_machine.first] = m_json; } - boost::nowide::ofstream c; - c.open(path_pid, std::ios::out | std::ios::trunc); - c << j.dump(1, '\t') << std::endl; - -#ifdef WIN32 - // WIN32 specific: The final "rename_file()" call is not safe in case of an application crash, there is no atomic "rename file" API - // provided by Windows (sic!). Therefore we save a MD5 checksum to be able to verify file corruption. In addition, - // we save the config file into a backup first before moving it to the final destination. - c << appconfig_md5_hash_line(j.dump(1, '\t')); -#endif - - c.close(); - if (c.fail()) { - BOOST_LOG_TRIVIAL(error) << "Failed to write new configuration to " << path_pid << "; aborting attempt to overwrite original configuration"; - return; - } - -#ifdef WIN32 - // Make a backup of the configuration file before copying it to the final destination. - std::string error_message; - std::string backup_path = (boost::format("%1%.bak") % path).str(); - // Copy configuration file with PID suffix into the configuration file with "bak" suffix. - if (copy_file(path_pid, backup_path, error_message, false) != SUCCESS) - BOOST_LOG_TRIVIAL(error) << "Copying from " << path_pid << " to " << backup_path << " failed. Failed to create a backup configuration."; -#endif - - // 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); - m_dirty = false; + const std::string config_str = j.dump(1, '\t'); + if (write_config_file(path, config_str + "\n", config_str)) + m_dirty = false; } #else -std::string AppConfig::load() +std::string AppConfig::load(bool read_only) { + // Keep another instance from replacing or restoring the file mid-read. + InstanceLock instance_lock(read_only ? std::string() : lock_path()); + if (instance_lock.locked()) + sweep_leftovers(); + // 1) Read the complete config file into a boost::property_tree. namespace pt = boost::property_tree; pt::ptree tree; @@ -1287,7 +1269,6 @@ 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(); - std::string path_pid = (boost::format("%1%.%2%") % path % get_current_pid()).str(); std::stringstream config_ss; if (m_mode == EAppMode::Editor) @@ -1323,39 +1304,40 @@ void AppConfig::save() // One empty line before the MD5 sum. config_ss << std::endl; - std::string config_str = config_ss.str(); - boost::nowide::ofstream c; - c.open(path_pid, std::ios::out | std::ios::trunc); - c << config_str; -#ifdef WIN32 - // WIN32 specific: The final "rename_file()" call is not safe in case of an application crash, there is no atomic "rename file" API - // provided by Windows (sic!). Therefore we save a MD5 checksum to be able to verify file corruption. In addition, - // we save the config file into a backup first before moving it to the final destination. - c << appconfig_md5_hash_line(config_str); -#endif - c.close(); - if (c.fail()) { - BOOST_LOG_TRIVIAL(error) << "Failed to write new configuration to " << path_pid << "; aborting attempt to overwrite original configuration"; - return; - } - -#ifdef WIN32 - // Make a backup of the configuration file before copying it to the final destination. - std::string error_message; - std::string backup_path = (boost::format("%1%.bak") % path).str(); - // Copy configuration file with PID suffix into the configuration file with "bak" suffix. - if (copy_file(path_pid, backup_path, error_message, false) != SUCCESS) - BOOST_LOG_TRIVIAL(error) << "Copying from " << path_pid << " to " << backup_path << " failed. Failed to create a backup configuration."; -#endif - - // 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); - m_dirty = false; + const std::string config_str = config_ss.str(); + if (write_config_file(path, config_str, config_str)) + m_dirty = false; } #endif +bool AppConfig::write_config_file(const std::string &path, std::string body, const std::string &checksum_source) +{ + // Everything before this is assembly; only the writes need the other instances kept out. + InstanceLock instance_lock(lock_path()); +#ifdef WIN32 + // WIN32 specific: the final replace is not safe in case of an application crash, there is no atomic "rename file" API + // provided by Windows (sic!). Therefore we save a MD5 checksum to be able to verify file corruption. In addition, + // we save the config file into a backup first before moving it to the final destination. + body += appconfig_md5_hash_line(checksum_source); +#endif + // The config was always replaced, never written in place, so a read-only one is replaced still. + // Not flushed to the device: the idle handler saves on the GUI thread after + // any change, and the rename already gives a complete old or new file. + 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() << "; trying again in 10 s"; + m_retry_save_at = std::chrono::steady_clock::now() + std::chrono::seconds(10); + return false; + } + m_retry_save_at = {}; +#ifdef WIN32 + // Written after the config, so the backup never holds a state that was not confirmed written. + const std::string backup_path = (boost::format("%1%.bak") % path).str(); + if (const std::error_code ec = write_file_atomically(backup_path, body, { false, /*replace_read_only=*/true })) + BOOST_LOG_TRIVIAL(error) << "Failed to write the backup configuration " << backup_path << ": " << ec.message(); +#endif + return true; +} + bool AppConfig::get_variant(const std::string &vendor, const std::string &model, const std::string &variant) const { const auto it_v = m_vendors.find(vendor); @@ -1844,6 +1826,20 @@ void AppConfig::reset_selections() } } +// Leftovers of interrupted writes in the directories the application owns +// that no preset scan covers: the data dir itself and cache/. +void AppConfig::sweep_leftovers() +{ + const boost::filesystem::path root(Slic3r::data_dir()); + remove_stale_temp_files(root); + remove_stale_temp_files(root / "cache"); +} + +std::string AppConfig::lock_path() +{ + return Slic3r::data_dir().empty() ? std::string() : config_path() + ".lock"; +} + std::string AppConfig::config_path() { #ifdef USE_JSON_CONFIG @@ -1880,7 +1876,7 @@ bool AppConfig::exists() std::string AppConfig::load_if_exists() { - return boost::filesystem::exists(loading_path()) ? load() : std::string(); + return boost::filesystem::exists(loading_path()) ? load(/*read_only=*/true) : std::string(); } }; // namespace Slic3r diff --git a/src/libslic3r/AppConfig.hpp b/src/libslic3r/AppConfig.hpp index 24a4c2069b..f55deb2d9c 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" @@ -121,14 +122,18 @@ public: // Load the slic3r.ini from a user profile directory (or a datadir, if configured). // Return an error string, or an empty string on success. - std::string load(); + std::string load(bool read_only = false); // Treat a missing config as default state; otherwise load it normally. + // The CLI's load: it never saves, so it takes no lock and creates no lock file. std::string load_if_exists(); // Store the slic3r.ini into a user profile directory (or a datadir, if configured). void save(); // 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; } @@ -338,6 +343,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; } @@ -448,8 +455,18 @@ private: // Preset for each machine MachineSettingMap m_printer_settings; + // Writes the assembled config text, and on Windows its checksum and a backup copy; false when the + // config itself could not be written, in which case the caller stays dirty and retries. `checksum_source` + // is the text load() will verify, which for the JSON config ends before the trailing newline. + bool write_config_file(const std::string &path, std::string body, const std::string &checksum_source); + + void sweep_leftovers(); + // Has any value been modified since the config.ini has been last saved or loaded? bool m_dirty; + // After a failed write, save_due() is false for the next ten seconds, so the + // idle handler does not repeat a hopeless write 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/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..0ba0e5e248 --- /dev/null +++ b/src/libslic3r/InstanceLock.cpp @@ -0,0 +1,175 @@ +#include "InstanceLock.hpp" + +#include +#include +#include +#include + +#include +#include +#include +#ifdef _WIN32 +#include +#include +#else +#include +#include +#include +#include +#endif + +namespace Slic3r { + +#ifdef _WIN32 +// LockFileEx, held by this handle alone. +using NativeFileLock = boost::interprocess::file_lock; +#else +// flock(2) rather than an fcntl lock: it belongs to this open file description, +// so any other code in the process that opens and closes the lock file, as a +// backup or an export walking the data dir might, cannot drop it. An fcntl +// lock would go with the first such close. +class NativeFileLock +{ +public: + explicit NativeFileLock(const char *path) : m_fd(::open(path, O_RDWR | O_CREAT | O_CLOEXEC, 0644)) + { + if (m_fd < 0) + throw std::system_error(errno, std::generic_category(), path); + } + ~NativeFileLock() { ::close(m_fd); } + bool try_lock() + { + if (::flock(m_fd, LOCK_EX | LOCK_NB) == 0) + return true; + if (errno == EWOULDBLOCK) + return false; + throw std::system_error(errno, std::generic_category(), "flock"); + } + void unlock() { ::flock(m_fd, LOCK_UN); } +private: + int m_fd; +}; +#endif + +// One slot per lock file, shared by every guard in the process: one lock +// object 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. The lock +// file is opened by the outermost guard and closed when it goes, so the file +// is never held open between guards: whatever is at the path is what gets +// locked, and a data dir can be removed once nothing is saving into it. The +// file is kept rather than deleted on release because the lock state lives in +// the kernel on the open file, and deleting it would let a third instance +// lock a fresh file while the second still holds the old one. +struct InstanceLock::Slot +{ + std::recursive_mutex mutex; + std::unique_ptr file_lock; + int depth{0}; + bool file_locked{false}; + // Cool-down after a guard could not take the file lock: until this point + // guards do not wait for it, and after a failed open or lock call do not + // touch the file at all. + std::chrono::steady_clock::time_point cooldown_until{}; + bool skip_file{false}; +}; + +InstanceLock::Slot &InstanceLock::slot_for(const std::string &lock_file_path) +{ + // Never freed: a save during static destruction still needs its slot. + static auto *registry_mutex = new std::mutex(); + 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(); + return *slot; +} + +// Starts the cool-down. Called with the slot mutex held. +void InstanceLock::defer(Slot &slot, bool skip_file, const std::string &reason) +{ + slot.cooldown_until = std::chrono::steady_clock::now() + cooldown; + slot.skip_file = skip_file; + BOOST_LOG_TRIVIAL(warning) << reason << "; proceeding without the lock for the next " << cooldown.count() << " ms"; +} + +// Creates the lock file if needed and opens it, or starts the cool-down. +// Called with the slot mutex held. +bool InstanceLock::open_lock_file(Slot &slot, const std::string &lock_file_path) +{ + try { +#ifdef _WIN32 + // The lock opens an existing file; created once, on the first miss. + const std::wstring wide_path = boost::nowide::widen(lock_file_path); + try { + slot.file_lock = std::make_unique(wide_path.c_str()); + } catch (const std::exception &) { + boost::nowide::ofstream(lock_file_path, std::ios::app).close(); + slot.file_lock = std::make_unique(wide_path.c_str()); + } +#else + slot.file_lock = std::make_unique(lock_file_path.c_str()); +#endif + return true; + } catch (const std::exception &e) { + defer(slot, true, "Cannot open lock file " + lock_file_path + ": " + e.what() + " (check its owner and permissions)"); + return false; + } +} + +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_guard = std::unique_lock(m_slot->mutex); + const auto now = std::chrono::steady_clock::now(); + const bool cooling_down = now < m_slot->cooldown_until; + if (m_slot->depth == 0 && ! (cooling_down && m_slot->skip_file) && open_lock_file(*m_slot, lock_file_path)) { + const auto deadline = now + timeout; + for (;;) { + try { + if (m_slot->file_lock->try_lock()) { + m_slot->file_locked = true; + m_slot->cooldown_until = {}; + break; + } + } catch (const std::exception &e) { + defer(*m_slot, true, "Cannot lock " + lock_file_path + ": " + e.what()); + break; + } + if (cooling_down) + break; + if (std::chrono::steady_clock::now() >= deadline) { + defer(*m_slot, false, "Another instance has held " + lock_file_path + " for over " + std::to_string(timeout.count()) + " ms"); + break; + } + std::this_thread::sleep_for(std::chrono::milliseconds(5)); + } + if (! m_slot->file_locked) + m_slot->file_lock.reset(); + } + // Counted last, so a throw above leaves the slot exactly as it was found. + ++ m_slot->depth; + m_locked = m_slot->file_locked; +} + +InstanceLock::~InstanceLock() +{ + if (m_slot == nullptr) + return; + if (-- m_slot->depth == 0 && m_slot->file_lock) { + if (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->file_lock.reset(); + } +} + +} // namespace Slic3r diff --git a/src/libslic3r/InstanceLock.hpp b/src/libslic3r/InstanceLock.hpp new file mode 100644 index 0000000000..3fe8e927a8 --- /dev/null +++ b/src/libslic3r/InstanceLock.hpp @@ -0,0 +1,61 @@ +#pragma once + +#include +#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`. The lock file is +// created on first use and kept; the OS releases the lock when its holder +// exits, so a crashed instance never leaves a stale lock behind. +// +// Best effort: when the lock file cannot be opened or locked, or another +// instance still holds it after `timeout`, the guard keeps only the in-process +// mutex, locked() reports false and the write proceeds, since a hung instance +// must never block another one from saving. For `cooldown` afterwards guards +// do not wait for the file, and after a failed open or lock call leave it +// alone. The wait for the in-process mutex is bounded only by the +// longest critical section, so a guard covers a few file operations and +// nothing slower. +// +// 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 guard +// must not be added around save_user_presets(), which takes the collection +// mutex through delete_preset(). +class InstanceLock +{ +public: + // Long against a critical section of milliseconds, short against the GUI + // thread, which is where most guards are taken. + static constexpr std::chrono::milliseconds default_timeout{1000}; + // Long enough that a holder stuck in a debugger does not cost a stall per + // save; mutable so tests can shorten it. + static inline 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); + ~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); + static bool open_lock_file(Slot &slot, const std::string &lock_file_path); + static void defer(Slot &slot, bool skip_file, const std::string &reason); + + Slot *m_slot{nullptr}; + std::unique_lock m_slot_guard; + bool m_locked{false}; +}; + +} // namespace Slic3r diff --git a/src/libslic3r/Preset.cpp b/src/libslic3r/Preset.cpp index 1002f5be89..d63c839f3a 100644 --- a/src/libslic3r/Preset.cpp +++ b/src/libslic3r/Preset.cpp @@ -48,6 +48,12 @@ #include "libslic3r.h" #include "Utils.hpp" +#include "InstanceLock.hpp" + +#include +#include +#include +#include #include "Time.hpp" #include "PlaceholderParser.hpp" #include "libslic3r/GCode/Thumbnails.hpp" @@ -104,6 +110,57 @@ std::string get_preset_canonical_name(const std::string &preset_bare_name, const } } +std::string user_presets_lock_path(bool read_only) +{ + return read_only || data_dir().empty() ? std::string() : (fs::path(data_dir()) / (PRESET_USER_DIR ".lock")).string(); +} + +// Removes a preset file the scan could not load, and its .info, under the lock. +// Without the lock, in a cool-down, the file may be another instance's fresh +// write that this scan merely raced, so it stays for the next scan to judge. +static void remove_preset_files(const std::string &preset_file, bool read_only) +{ + if (read_only) + return; + const std::string lock_path = user_presets_lock_path(); + InstanceLock instance_lock(lock_path); + if (! lock_path.empty() && ! instance_lock.locked()) { + BOOST_LOG_TRIVIAL(warning) << "Leaving unreadable preset " << preset_file << " in place: the instance lock is not held"; + return; + } + boost::system::error_code ec; + fs::path file_path(preset_file); + fs::remove(file_path, ec); + file_path.replace_extension(".info"); + fs::remove(file_path, ec); +} + +void remove_directory_tree_locked(const boost::filesystem::path &dir) +{ + boost::system::error_code ec; + // Set aside under cache/, which no preset scan reads, named so the sweep + // there removes it if this process dies before remove_all() is through. + static std::atomic counter{0}; + const fs::path bin = data_dir().empty() ? fs::path() : fs::path(data_dir()) / "cache"; + const fs::path doomed = bin / ("removing." + std::to_string(get_current_pid()) + "." + std::to_string(counter++)); + { + InstanceLock instance_lock(user_presets_lock_path()); + if (! fs::exists(dir, ec)) + return; + if (! bin.empty()) + fs::create_directories(bin, ec); + if (bin.empty() || (fs::rename(dir, doomed, ec), ec)) { + // Cannot be set aside: the slow way, still under the lock. + fs::remove_all(dir, ec); + return; + } + // A renamed tree keeps its old modification time; the sweep must see + // it as set aside just now, not as hours-old leftovers. + fs::last_write_time(doomed, std::time(nullptr), ec); + } + fs::remove_all(doomed, ec); +} + std::string get_preset_bare_name(const std::string &canonical_name) { const auto pos = canonical_name.find_last_of('/'); @@ -646,18 +703,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 +725,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,12 +762,16 @@ void Preset::save(DynamicPrintConfig* parent_config) else from_str = std::string("Default"); - boost::filesystem::create_directories(fs::path(this->file).parent_path()); const std::string bare_name = get_preset_bare_name(this->name); + // What gets written: the diff against the parent, the config plus its + // filament id, or the config as is. Built before the lock is taken so the + // exclusive window covers only the file writes. + DynamicPrintConfig temp_config; + const DynamicPrintConfig *to_save = &this->config; + //BBS: only save difference if it has parent if (parent_config) { - DynamicPrintConfig temp_config; std::vector dirty_options = config.diff(*parent_config); std::string extruder_id_name, extruder_variant_name; @@ -743,13 +807,22 @@ void Preset::save(DynamicPrintConfig* parent_config) opt_dst->set(opt_src); } } - temp_config.save_to_json(this->file, bare_name, from_str, this->version.to_string()); + to_save = &temp_config; } else if (!filament_id.empty() && inherits().empty()) { - DynamicPrintConfig temp_config = config; + temp_config = config; temp_config.set_key_value(BBL_JSON_KEY_FILAMENT_ID, new ConfigOptionString(filament_id)); - temp_config.save_to_json(this->file, bare_name, from_str, this->version.to_string()); - } else { - this->config.save_to_json(this->file, bare_name, from_str, this->version.to_string()); + 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()); + if (const std::error_code ec = write_file_atomically(this->file, json.str())) { + // No .info either: one without its preset reads as a cloud deletion request. + BOOST_LOG_TRIVIAL(error) << __FUNCTION__ << ": failed to save " << this->file << ": " << ec.message(); + return; } BOOST_LOG_TRIVIAL(info) << __FUNCTION__ << " save config for: " << this->name << " and filament_id: " << filament_id << " and base_id: " << this->base_id; @@ -770,6 +843,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)); @@ -1704,9 +1778,22 @@ 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); + // The lock is re-taken every few files: each outermost guard opens and + // closes the lock file, and a save on another thread or in another + // instance must never wait for the whole scan. + constexpr int files_per_lock = 32; + std::optional instance_lock; + int files_under_lock = files_per_lock; //BBS: change to json format for (auto &dir_entry : boost::filesystem::directory_iterator(dir)) { + if (++ files_under_lock > files_per_lock) { + instance_lock.emplace(user_presets_lock_path(read_only)); + files_under_lock = 1; + } + // Leftovers of an interrupted write go on this pass, not a second listing. + if (instance_lock->locked() && remove_if_stale_leftover(dir_entry)) + continue; std::string file_name = dir_entry.path().filename().string(); //if (Slic3r::is_ini_file(dir_entry)) { if (Slic3r::is_json_file(file_name)) { @@ -1725,30 +1812,26 @@ void PresetCollection::load_presets( preset.file = dir_entry.path().string(); // Load the preset file, apply preset values on top of defaults. try { + DynamicPrintConfig config; + std::map key_values; + std::string reason; + ConfigSubstitutions config_substitutions; 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); - if (! config_substitutions.empty()) - substitutions.push_back({ preset.name, m_type, PresetConfigSubstitutions::Source::UserFile, preset.file, std::move(config_substitutions) }); + 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); + remove_preset_files(preset.file, read_only); 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) }); std::string version_str = key_values[BBL_JSON_KEY_VERSION]; boost::optional version = Semver::parse(version_str); @@ -1832,23 +1915,13 @@ 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(); - 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); + remove_preset_files(preset.file, read_only); //throw Slic3r::RuntimeError(std::string("The selected preset cannot be loaded: ") + preset.file + "\n\tReason: " + err.what()); } catch (const std::runtime_error &err) { ++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()); - 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); + remove_preset_files(preset.file, read_only); } if (preset_loaded_fn != nullptr) @@ -1862,6 +1935,7 @@ void PresetCollection::load_presets( } } } + instance_lock.reset(); if (presets_loaded.size() > 0) m_presets.insert(m_presets.end(), std::make_move_iterator(presets_loaded.begin()), std::make_move_iterator(presets_loaded.end())); sort_presets(); @@ -4201,8 +4275,15 @@ void PhysicalPrinter::update_preset_names_in_config() } } +void PhysicalPrinter::save(DynamicPrintConfig* /* parent_config */) +{ + InstanceLock instance_lock(user_presets_lock_path()); + this->config.save_to_json(this->file, std::string("Physical_Printer"), std::string("User"), std::string(SLIC3R_VERSION)); +} + 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 +4415,7 @@ void PhysicalPrinterCollection::load_printers( continue; } try { + InstanceLock instance_lock(user_presets_lock_path()); PhysicalPrinter printer(name, this->default_config()); printer.file = dir_entry.path().string(); // Load the preset file, apply preset values on top of defaults. @@ -4526,7 +4608,10 @@ bool PhysicalPrinterCollection::delete_printer(const std::string& name) const PhysicalPrinter& printer = *it; // Erase the preset file. - boost::nowide::remove(printer.file.c_str()); + { + InstanceLock instance_lock(user_presets_lock_path()); + boost::nowide::remove(printer.file.c_str()); + } m_printers.erase(it); return true; } @@ -4538,7 +4623,10 @@ bool PhysicalPrinterCollection::delete_selected_printer() const PhysicalPrinter& printer = this->get_selected_printer(); // Erase the preset file. - boost::nowide::remove(printer.file.c_str()); + { + InstanceLock instance_lock(user_presets_lock_path()); + boost::nowide::remove(printer.file.c_str()); + } // Remove the preset from the list. m_printers.erase(m_printers.begin() + m_idx_selected); // unselect all printers diff --git a/src/libslic3r/Preset.hpp b/src/libslic3r/Preset.hpp index c66518e29d..6c6ba9d77b 100644 --- a/src/libslic3r/Preset.hpp +++ b/src/libslic3r/Preset.hpp @@ -484,6 +484,18 @@ 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, and +// for a read-only load (the CLI), which never rewrites or deletes and may run +// many jobs on one data dir. +std::string user_presets_lock_path(bool read_only = false); + +// Removes a directory tree under the user preset lock without holding the lock +// for the removal itself: the tree is moved into cache/ under the lock, in one +// step, and deleted afterwards, so a save waiting on the lock waits +// milliseconds rather than for a tree of files to go. +void remove_directory_tree_locked(const boost::filesystem::path &dir); + // 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()); @@ -1053,7 +1065,7 @@ public: //BBS: change to json format //void save() { this->config.save(this->file); } - void save(DynamicPrintConfig* parent_config) { this->config.save_to_json(this->file, std::string("Physical_Printer"), std::string("User"), std::string(SLIC3R_VERSION)); } + void save(DynamicPrintConfig* parent_config); void save(const std::string& file_name_from, const std::string& file_name_to); void update_from_preset(const Preset& preset); diff --git a/src/libslic3r/PresetBundle.cpp b/src/libslic3r/PresetBundle.cpp index dc731b63c4..443e6662eb 100644 --- a/src/libslic3r/PresetBundle.cpp +++ b/src/libslic3r/PresetBundle.cpp @@ -1,3 +1,4 @@ +#include #include #include #include @@ -12,6 +13,7 @@ #include "libslic3r.h" #include "I18N.hpp" #include "Utils.hpp" +#include "InstanceLock.hpp" #include "LocalesUtils.hpp" #include "Model.hpp" #include "TriangleSelector.hpp" @@ -1227,6 +1229,15 @@ PresetsConfigSubstitutions PresetBundle::load_user_presets(std::string user, For const auto user_load_t0 = std::chrono::steady_clock::now(); + // Reads one bundle's metadata under the lock, per file, so the lock is never + // held when bundles.WriteLock() is taken afterwards. + auto load_bundle_metadata = [read_only](const fs::path &bundle_dir, const fs::path &metadata_file, BundleMetadata &metadata) { + InstanceLock instance_lock(user_presets_lock_path(read_only)); + if (instance_lock.locked()) + remove_stale_temp_files(bundle_dir); + return metadata.load_from_json(metadata_file.string()); + }; + // Load bundle metadata from _local directory first fs::path local_dir(folder / PRESET_LOCAL_DIR); if (fs::exists(local_dir)) { @@ -1240,7 +1251,7 @@ PresetsConfigSubstitutions PresetBundle::load_user_presets(std::string user, For if (!fs::exists(metadata_file)) continue; BundleMetadata metadata; - if (!metadata.load_from_json(metadata_file.string())) continue; + if (!load_bundle_metadata(entry.path(), metadata_file, metadata)) continue; metadata.print_presets.clear(); metadata.filament_presets.clear(); metadata.printer_presets.clear(); @@ -1275,7 +1286,7 @@ PresetsConfigSubstitutions PresetBundle::load_user_presets(std::string user, For if (!fs::exists(metadata_file)) continue; BundleMetadata metadata; - if (!metadata.load_from_json(metadata_file.string())) continue; + if (!load_bundle_metadata(entry.path(), metadata_file, metadata)) continue; metadata.print_presets.clear(); metadata.filament_presets.clear(); metadata.printer_presets.clear(); @@ -1609,10 +1620,13 @@ PresetsConfigSubstitutions PresetBundle::import_presets(std::vector if (ec) BOOST_LOG_TRIVIAL(error) << __FUNCTION__ << " create directory failed: " << ec.message(); //create temp folder //std::string user_default_temp_dir = data_dir() + "/" + PRESET_USER_DIR + "/" + DEFAULT_USER_FOLDER_NAME + "/" + "temp"; - fs::path temp_folder(configs_folder / "temp"); + // Under cache/, per process and per import, so two instances importing + // at once do not clear each other's extraction, no preset scan reads it, + // and the sweep there removes it if this import dies partway. + static std::atomic import_counter{0}; + fs::path temp_folder(fs::path(data_dir()) / "cache" / ("import." + std::to_string(get_current_pid()) + "." + std::to_string(import_counter++))); std::string user_default_temp_dir = temp_folder.make_preferred().string(); - if (fs::exists(temp_folder)) fs::remove_all(temp_folder); - fs::create_directory(temp_folder, ec); + fs::create_directories(temp_folder, ec); if (ec) BOOST_LOG_TRIVIAL(error) << __FUNCTION__ << " create directory failed: " << ec.message(); file = boost::filesystem::path(file).make_preferred().string(); @@ -1624,6 +1638,9 @@ PresetsConfigSubstitutions PresetBundle::import_presets(std::vector status = mz_zip_reader_init_cfile(&zip_archive, zipFile, 0, MZ_ZIP_FLAG_CASE_SENSITIVE | MZ_ZIP_FLAG_IGNORE_PATH); if (MZ_FALSE == status) { BOOST_LOG_TRIVIAL(info) << __FUNCTION__ << " Failed to initialize reader ZIP archive"; + if (zipFile != nullptr) + std::fclose(zipFile); + fs::remove_all(temp_folder, ec); return substitutions; } else { BOOST_LOG_TRIVIAL(info) << __FUNCTION__ << " Success to initialize reader ZIP archive"; @@ -2231,10 +2248,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; - fs::path folder(dir_user_presets); - if (fs::exists(folder)) { - fs::remove_all(folder); - } + remove_directory_tree_locked(fs::path(dir_user_presets)); } void PresetBundle::update_system_preset_setting_ids(std::map>& system_presets) @@ -7932,9 +7946,13 @@ bool BundleMetadata::save_to_json(const std::string& path) const 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(); + 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; + } + 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/PresetCacheFormat.cpp b/src/libslic3r/PresetCacheFormat.cpp index accebba7b4..eec38f5264 100644 --- a/src/libslic3r/PresetCacheFormat.cpp +++ b/src/libslic3r/PresetCacheFormat.cpp @@ -400,46 +400,24 @@ bool write_cache_blob(const std::string& path, const std::string& blob) { boost::crc_32_type crc; crc.process_bytes(blob.data(), blob.size()); - // Written beside the target and moved into place, as AppConfig::save does: - // a cache is truncated and rewritten in full, so a write that dies partway - // would otherwise leave a header claiming more body than the file holds. - // The PID suffix also keeps two instances writing the same vendor from - // interleaving. - const std::string tmp_path = path + "." + std::to_string(get_current_pid()) + ".tmp"; + // Written beside the target and moved into place: a cache is truncated and + // rewritten in full, so a write that dies partway would otherwise leave a + // header claiming more body than the file holds. try { boost::filesystem::create_directories(boost::filesystem::path(path).parent_path()); - { - boost::nowide::ofstream ofs(tmp_path, std::ios::binary | std::ios::trunc); - if (!ofs.is_open()) { - BOOST_LOG_TRIVIAL(warning) << "VendorCacheFile: cannot open for writing: " << tmp_path; - return false; - } - CacheFileHeader fhdr; - fhdr.magic = CACHE_MAGIC; - fhdr.version = CACHE_VERSION; - fhdr.data_size = static_cast(blob.size()); - fhdr.crc32 = crc.checksum(); - ofs.write(reinterpret_cast(&fhdr), sizeof(fhdr)); - ofs.write(blob.data(), static_cast(blob.size())); - ofs.close(); // flush; close() raises failbit on error - if (! ofs.good()) { - BOOST_LOG_TRIVIAL(warning) << "VendorCacheFile: write failed (" << tmp_path << ")"; - boost::system::error_code ec; - boost::filesystem::remove(tmp_path, ec); - return false; - } - } - if (const std::error_code ec = rename_file(tmp_path, path)) { - BOOST_LOG_TRIVIAL(warning) << "VendorCacheFile: could not move " << tmp_path << " into place: " << ec.message(); - boost::system::error_code rm; - boost::filesystem::remove(tmp_path, rm); + CacheFileHeader fhdr; + fhdr.magic = CACHE_MAGIC; + fhdr.version = CACHE_VERSION; + fhdr.data_size = static_cast(blob.size()); + fhdr.crc32 = crc.checksum(); + const std::string_view header(reinterpret_cast(&fhdr), sizeof(fhdr)); + if (const std::error_code ec = write_file_atomically(path, { header, std::string_view(blob) }, { /*binary=*/true })) { + BOOST_LOG_TRIVIAL(warning) << "VendorCacheFile: write failed (" << path << "): " << ec.message(); return false; } return true; } catch (const std::exception& e) { BOOST_LOG_TRIVIAL(warning) << "VendorCacheFile: write failed (" << path << "): " << e.what(); - boost::system::error_code ec; - boost::filesystem::remove(tmp_path, ec); return false; } } diff --git a/src/libslic3r/Utils.hpp b/src/libslic3r/Utils.hpp index b21da72fc8..84259a7721 100644 --- a/src/libslic3r/Utils.hpp +++ b/src/libslic3r/Utils.hpp @@ -8,6 +8,8 @@ #include #include #include +#include +#include #include #include @@ -224,6 +226,38 @@ 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); +struct WriteFileOptions +{ + // Bytes as given; otherwise text mode, where Windows writes CRLF. + bool binary = false; + // Replace a target this process may not write, for a file that was always + // replaced rather than written, such as the app config; otherwise such a + // target is refused, as an in-place write would have been. + bool replace_read_only = false; +}; + +// Write `chunks`, in order, 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 is removed on failure and an existing +// target keeps its permissions. A 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 symlink is followed and the file it names is +// replaced. On Windows a reader holding the target open without sharing its +// deletion, which the C runtime does not, makes the replace fall back to the +// in-place write too, so an unlocked reader there can still see a partial +// file. +extern std::error_code write_file_atomically(const std::string &path, std::initializer_list chunks, WriteFileOptions options = {}); +inline std::error_code write_file_atomically(const std::string &path, const std::string &content, WriteFileOptions options = {}) + { return write_file_atomically(path, { std::string_view(content) }, options); } +// Remove `entry` when it is a leftover of an interrupted write at least an hour +// old: a `...tmp` file of write_file_atomically(), or a +// `removing..` or `import..` tree of a removal or bundle import; +// only names starting with `name_prefix` when it is given. For a directory +// listing that already visits every entry; returns whether it was removed. +extern bool remove_if_stale_leftover(const boost::filesystem::directory_entry &entry, const std::string &name_prefix = std::string()); +// The same over every entry of `dir`. Returns how many were removed. +extern size_t remove_stale_temp_files(const boost::filesystem::path &dir, const std::string &name_prefix = std::string()); enum CopyFileResult { SUCCESS = 0, diff --git a/src/libslic3r/utils.cpp b/src/libslic3r/utils.cpp index 9def5dad17..5f1b129cbe 100644 --- a/src/libslic3r/utils.cpp +++ b/src/libslic3r/utils.cpp @@ -9,6 +9,9 @@ #include #include #include +#include +#include +#include #include #include #include @@ -702,13 +705,200 @@ namespace WindowsSupport std::error_code rename_file(const std::string &from, const std::string &to) { #ifdef _WIN32 + // Retries and moves an open destination aside itself. 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. + if (boost::nowide::rename(from.c_str(), to.c_str()) == 0) + return {}; + const int err = errno; + // Some mounts (sshfs, gvfs, MTP and a few SMB setups) refuse to replace an + // existing target in one step, each with the error it sees fit; every error + // is worth the remove-then-rename this always did, except the ones no retry + // can help: nothing at the source, a different device, or a directory where + // a file was expected and the reverse. + const bool worth_retrying = err != ENOENT && err != EXDEV && err != ENOTDIR && err != EISDIR; + if (worth_retrying && boost::nowide::remove(to.c_str()) == 0 && boost::nowide::rename(from.c_str(), to.c_str()) == 0) + return {}; + return std::make_error_code(static_cast(err)); #endif } +// Whether this process may open `path` for writing. +static bool is_writable(const std::string &path) +{ +#ifdef _WIN32 + // _waccess() sees only the read-only attribute; an open for writing sees + // ACLs too. Another process merely holding the file open is not a refusal: + // the rename that follows moves an open destination aside. + 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) { + const DWORD err = ::GetLastError(); + return err == ERROR_SHARING_VIOLATION || err == ERROR_LOCK_VIOLATION; + } + ::CloseHandle(handle); + return true; +#else + return ::access(path.c_str(), W_OK) == 0; +#endif +} + +static std::error_code write_whole_file(const std::string &path, std::initializer_list chunks, bool binary) +{ + errno = 0; + FILE *file = boost::nowide::fopen(path.c_str(), binary ? "wb" : "w"); + if (file == nullptr) + return std::make_error_code(errno != 0 ? static_cast(errno) : std::errc::io_error); + bool ok = true; + for (const std::string_view chunk : chunks) + ok = ok && std::fwrite(chunk.data(), 1, chunk.size(), file) == chunk.size(); + ok = ok && std::fflush(file) == 0; + const int err = ok ? 0 : errno; + ok = std::fclose(file) == 0 && ok; + if (ok) + return {}; + return std::make_error_code(err != 0 ? static_cast(err) : std::errc::io_error); +} + +// The in-place fallback truncates the target, so two threads of this process +// on the same file must not both be in it. One mutex for all such writes: they +// are the rare case. Never freed, like the InstanceLock registry, so a save +// during static destruction still finds it. +static std::error_code write_in_place(const std::string &path, std::initializer_list chunks, bool binary) +{ + static auto *mutex = new std::mutex(); + std::lock_guard guard(*mutex); + return write_whole_file(path, chunks, binary); +} + +std::error_code write_file_atomically(const std::string &path, std::initializer_list chunks, WriteFileOptions options) +{ + const bool binary = options.binary; + boost::system::error_code bec; + const boost::filesystem::file_status target = boost::filesystem::symlink_status(path, bec); + const bool target_exists = ! bec && boost::filesystem::exists(target); + if (target_exists && boost::filesystem::is_symlink(target)) { + // A config or preset kept in a dotfiles repository: the link stays, + // the file it points to is replaced like any other. + const boost::filesystem::path resolved = boost::filesystem::canonical(path, bec); + if (! bec && boost::filesystem::is_regular_file(resolved, bec)) + return write_file_atomically(resolved.string(), chunks, options); + } + if (target_exists && ! boost::filesystem::is_regular_file(target)) + return write_in_place(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 && ! is_writable(path)) { + if (! options.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. + static std::atomic counter{0}; + const std::string tmp_path = path + "." + std::to_string(get_current_pid()) + "." + std::to_string(counter++) + ".tmp"; + 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, or a name the longer temporary pushes past a path limit, which + // Windows reports as the path not being found. + const bool parent_exists = boost::filesystem::is_directory(boost::filesystem::path(path).parent_path(), bec); + const bool only_the_temporary_failed = (target_exists && ec == std::errc::permission_denied) || + (parent_exists && ec == std::errc::no_such_file_or_directory) || + ec == std::errc::filename_too_long; + if (only_the_temporary_failed) { + BOOST_LOG_TRIVIAL(warning) << "Cannot create a temporary beside " << path << " (" << ec.message() << "); writing in place"; + return write_in_place(path, chunks, binary); + } + return ec; + } +#ifndef _WIN32 + // 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 + if (const std::error_code ec = rename_file(tmp_path, path)) { + boost::nowide::remove(tmp_path.c_str()); + // A reader on Windows holding the target open without FILE_SHARE_DELETE, + // or a mount that cannot replace a file 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_in_place(path, chunks, binary); + } + return {}; +} + +// The shapes this code leaves behind: ...tmp from +// write_file_atomically(), and the directories removing.. from +// remove_directory_tree_locked() and import.. from a bundle import. +enum class Leftover { None, Temporary, DoomedTree }; + +static bool digit_segments_before(const std::string &name, size_t end, int count) +{ + for (int segment = 0; segment < count; ++ 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; +} + +static Leftover classify_leftover(const std::string &name, const std::string &name_prefix) +{ + if (name.compare(0, name_prefix.size(), name_prefix) != 0) + return Leftover::None; + static const std::string tmp_suffix = ".tmp"; + if (name.size() > tmp_suffix.size() && name.compare(name.size() - tmp_suffix.size(), tmp_suffix.size(), tmp_suffix) == 0) + return digit_segments_before(name, name.size() - tmp_suffix.size(), 2) ? Leftover::Temporary : Leftover::None; + for (const char *tree_prefix : { "removing.", "import." }) + if (name.compare(0, std::strlen(tree_prefix), tree_prefix) == 0 && digit_segments_before(name, name.size(), 2)) + return Leftover::DoomedTree; + return Leftover::None; +} + +bool remove_if_stale_leftover(const boost::filesystem::directory_entry &entry, const std::string &name_prefix) +{ + // The name test first: it is free, the stat is not. + const Leftover kind = classify_leftover(entry.path().filename().string(), name_prefix); + if (kind == Leftover::None) + return false; + boost::system::error_code ec; + const boost::filesystem::file_status status = entry.symlink_status(ec); + if (kind == Leftover::DoomedTree ? ! boost::filesystem::is_directory(status) : ! boost::filesystem::is_regular_file(status)) + return false; + // An instance that gave up waiting for the lock writes unlocked by design, + // 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 written = boost::filesystem::last_write_time(entry.path(), ec); + if (ec || std::time(nullptr) - written < stale_age) + return false; + if (kind == Leftover::DoomedTree ? boost::filesystem::remove_all(entry.path(), ec) > 0 : boost::filesystem::remove(entry.path(), ec)) { + BOOST_LOG_TRIVIAL(info) << "Removed stale leftover " << entry.path(); + return true; + } + return false; +} + +size_t remove_stale_temp_files(const boost::filesystem::path &dir, const std::string &name_prefix) +{ + size_t removed = 0; + boost::system::error_code ec; + for (boost::filesystem::directory_iterator it(dir, ec), end; ! ec && it != end; it.increment(ec)) + if (remove_if_stale_leftover(*it, name_prefix)) + ++ removed; + return removed; +} + #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..bb12d9a452 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" @@ -3531,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(); }); @@ -7623,8 +7624,7 @@ 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); + remove_directory_tree_locked(bundle_folder); preset_bundle->bundles.WriteLock(); preset_bundle->bundles.m_bundles.erase(bundle.id); @@ -8889,6 +8889,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; } @@ -8951,15 +8952,19 @@ void GUI_App::scan_orphaned_info_files() fs::path preset_file = info_file; preset_file.replace_extension(".json"); - // If .json doesn't exist, .info is orphaned - if (!fs::exists(preset_file)) { - // Extract setting_id from .info file - std::string setting_id = extract_setting_id_from_info(info_file.string()); - if (!setting_id.empty()) { - // Add to need_delete_presets - delete_preset_from_cloud(setting_id, info_file.string()); - BOOST_LOG_TRIVIAL(info) << "Found orphaned .info file on startup: " << info_file.string(); - } + // If .json doesn't exist, .info is orphaned. Read under the lock, so a + // remove_files() in another instance is seen whole or not at all; the + // delete queue's own mutex is taken after the lock is released. + std::string setting_id; + { + InstanceLock instance_lock(user_presets_lock_path()); + if (!fs::exists(preset_file)) + setting_id = extract_setting_id_from_info(info_file.string()); + } + if (!setting_id.empty()) { + // Add to need_delete_presets + delete_preset_from_cloud(setting_id, info_file.string()); + BOOST_LOG_TRIVIAL(info) << "Found orphaned .info file on startup: " << info_file.string(); } } if (ec) diff --git a/src/slic3r/GUI/WebGuideDialog.cpp b/src/slic3r/GUI/WebGuideDialog.cpp index 78030c68da..6632f0d8b3 100644 --- a/src/slic3r/GUI/WebGuideDialog.cpp +++ b/src/slic3r/GUI/WebGuideDialog.cpp @@ -1481,9 +1481,7 @@ bool GuideFrame::BuildProfileDataFromVendors() return false; // Written through a temp file and moved into place, as the preset caches - // are: half a cache must never be readable, and the PID suffix keeps two - // instances from interleaving on one temp file. - const std::string tmp_path = cache_file.string() + "." + std::to_string(get_current_pid()) + ".tmp"; + // are: half a cache must never be readable. try { json out; out["format"] = 1; @@ -1492,18 +1490,9 @@ bool GuideFrame::BuildProfileDataFromVendors() for (const char* key : { "model", "machine", "filament", "process" }) profile[key] = m_ProfileJson[key]; boost::filesystem::create_directories(cache_file.parent_path()); - { - boost::nowide::ofstream ofs(tmp_path, std::ios::binary | std::ios::trunc); - ofs << out.dump(-1, ' ', false, json::error_handler_t::ignore); - ofs.close(); - if (! ofs.good()) - throw std::runtime_error("write failed"); - } - if (const std::error_code ec = rename_file(tmp_path, cache_file.string())) + if (const std::error_code ec = write_file_atomically(cache_file.string(), out.dump(-1, ' ', false, json::error_handler_t::ignore), { /*binary=*/true })) throw std::runtime_error(ec.message()); } catch (const std::exception& e) { - boost::system::error_code rm; - boost::filesystem::remove(tmp_path, rm); BOOST_LOG_TRIVIAL(warning) << "GuideFrame: could not write the profile data cache: " << e.what(); } return true; diff --git a/src/slic3r/Utils/3DPrinterOS.cpp b/src/slic3r/Utils/3DPrinterOS.cpp index 503dbe63e3..081afb667c 100755 --- a/src/slic3r/Utils/3DPrinterOS.cpp +++ b/src/slic3r/Utils/3DPrinterOS.cpp @@ -2,6 +2,7 @@ #include #include +#include #include #include #include @@ -580,9 +581,10 @@ bool C3DPrinterOS::save_api_session(const std::string &session, const std::strin j.put("session", session); j.put("email", email); try { - auto temp_path = m_api_session_file_path + ".tmp"; - pt::write_json(temp_path, j); - boost::filesystem::rename(temp_path, m_api_session_file_path); + std::ostringstream json; + pt::write_json(json, j); + if (const std::error_code ec = write_file_atomically(m_api_session_file_path, json.str())) + throw std::system_error(ec); } catch (const std::exception &err) { BOOST_LOG_TRIVIAL(error) << __FUNCTION__ << ": failed to write json to file. Path = " << m_api_session_file_path diff --git a/src/slic3r/Utils/OrcaCloudServiceAgent.cpp b/src/slic3r/Utils/OrcaCloudServiceAgent.cpp index 5d129a490e..1347c1151f 100644 --- a/src/slic3r/Utils/OrcaCloudServiceAgent.cpp +++ b/src/slic3r/Utils/OrcaCloudServiceAgent.cpp @@ -1475,15 +1475,8 @@ void OrcaCloudServiceAgent::save_sync_state() if (sync_state_path.empty()) return; - try { - std::string tmp_path = sync_state_path + ".tmp"; - std::ofstream ofs(tmp_path, std::ios::out | std::ios::trunc); - if (ofs.good()) { - ofs << std::to_string(sync_state.last_sync_timestamp); - ofs.close(); - boost::filesystem::rename(tmp_path, sync_state_path); - } - } catch (...) {} + if (const std::error_code ec = write_file_atomically(sync_state_path, std::to_string(sync_state.last_sync_timestamp))) + BOOST_LOG_TRIVIAL(warning) << "OrcaCloudServiceAgent: failed to save the sync state: " << ec.message(); } void OrcaCloudServiceAgent::clear_sync_state() @@ -1572,22 +1565,10 @@ void OrcaCloudServiceAgent::persist_user_secret(const std::string& secret) wxFileName::Mkdir(path.GetPath(), wxS_DIR_DEFAULT, wxPATH_MKDIR_FULL); } - const std::string tmp_path = secret_fallback_path + ".tmp"; - std::ofstream ofs(tmp_path, std::ios::out | std::ios::trunc | std::ios::binary); - if (ofs.good()) { - ofs << signed_payload; - ofs.flush(); - ofs.close(); - - if (wxRenameFile(wxString::FromUTF8(tmp_path.c_str()), wxString::FromUTF8(secret_fallback_path.c_str()), true)) { - stored = true; - } else { - wxRemoveFile(wxString::FromUTF8(tmp_path.c_str())); - BOOST_LOG_TRIVIAL(warning) << "OrcaCloudServiceAgent: failed to atomically replace user secret file"; - } - } else { - BOOST_LOG_TRIVIAL(warning) << "OrcaCloudServiceAgent: cannot open user secret file for write - " << secret_fallback_path; - } + if (const std::error_code ec = write_file_atomically(secret_fallback_path, signed_payload, { /*binary=*/true })) + BOOST_LOG_TRIVIAL(warning) << "OrcaCloudServiceAgent: cannot write user secret file " << secret_fallback_path << ": " << ec.message(); + else + stored = true; } else { // Use wxSecretStore only wxSecretStore store = wxSecretStore::GetDefault(); diff --git a/src/slic3r/plugin/PluginConfig.cpp b/src/slic3r/plugin/PluginConfig.cpp index a6f0ead18b..011c56c899 100644 --- a/src/slic3r/plugin/PluginConfig.cpp +++ b/src/slic3r/plugin/PluginConfig.cpp @@ -204,6 +204,7 @@ nlohmann::json CapabilityConfigDocument::root_json() const void PluginConfig::load() { const std::string path = plugin_config_file(); + remove_stale_temp_files(boost::filesystem::path(path).parent_path(), boost::filesystem::path(path).filename().string()); std::lock_guard lock(m_mutex); m_document = CapabilityConfigDocument(); @@ -249,21 +250,10 @@ bool PluginConfig::save() return false; } - // Write to a PID-suffixed file and rename it into place, so a crash mid-write cannot truncate an - // existing config. Same approach as AppConfig::save(). - const std::string path_pid = (boost::format("%1%.%2%") % path % get_current_pid()).str(); - - boost::nowide::ofstream file; - file.open(path_pid, std::ios::out | std::ios::trunc); - file << root.dump(1, '\t') << std::endl; - file.close(); - if (file.fail()) { - BOOST_LOG_TRIVIAL(error) << "PluginConfig: failed to write " << path_pid << "; keeping the existing config"; - return false; - } - - if (const std::error_code rename_ec = rename_file(path_pid, path)) { - BOOST_LOG_TRIVIAL(error) << "PluginConfig: failed to move " << path_pid << " onto " << path << ": " << rename_ec.message(); + // Written beside the target and moved into place, so a crash mid-write cannot truncate an + // existing config. + if (const std::error_code ec = write_file_atomically(path, root.dump(1, '\t') + "\n")) { + BOOST_LOG_TRIVIAL(error) << "PluginConfig: failed to write " << path << ": " << ec.message() << "; keeping the existing config"; return false; } 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..125023cd02 --- /dev/null +++ b/tests/libslic3r/test_instance_lock.cpp @@ -0,0 +1,202 @@ +#include + +#include +#include +#include + +#include + +#include "libslic3r/InstanceLock.hpp" +#include "test_utils.hpp" + +#ifndef _WIN32 +#include +#include +#include +#include +#endif + +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"); + const std::string path = lock_file.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()); + // Well inside the timeout it would otherwise have waited out; loose enough for a loaded runner. + REQUIRE(std::chrono::steady_clock::now() - started < 4000ms); +} + +TEST_CASE("InstanceLock nests within one thread", "[InstanceLock]") +{ + ScopedTemporaryFile lock_file(".lock"); + const std::string path = lock_file.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 retries a lock file it could not open once the cool-down passes", "[InstanceLock]") +{ + ScopedTemporaryDir dir; + const std::string path = (dir.path() / "later" / "shared.lock").string(); + ScopedStaticValue cooldown(InstanceLock::cooldown, 300ms); + + bool before_dir, during_cooldown, after_cooldown; + const auto started = std::chrono::steady_clock::now(); + { + InstanceLock lock(path, 100ms); + before_dir = lock.locked(); + } + boost::filesystem::create_directories(dir.path() / "later"); + { + InstanceLock lock(path, 100ms); + during_cooldown = lock.locked(); + } + const bool second_guard_inside_cooldown = std::chrono::steady_clock::now() - started < InstanceLock::cooldown; + std::this_thread::sleep_for(400ms); + { + InstanceLock lock(path, 100ms); + after_cooldown = lock.locked(); + } + + REQUIRE_FALSE(before_dir); + // A loaded runner may take longer than the cool-down to get here; then the + // second guard legitimately retried, so only assert when the timing held. + if (second_guard_inside_cooldown) + REQUIRE_FALSE(during_cooldown); + 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()); + // Each outermost guard opens the file afresh, so the deleted path is back. + REQUIRE(boost::filesystem::exists(path)); +} + +TEST_CASE("InstanceLock serialises the threads of one process", "[InstanceLock]") +{ + ScopedTemporaryFile lock_file(".lock"); + const std::string path = lock_file.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 flock, which a child process +// takes here directly; LockFileEx backs the guard on Windows, but spawning a +// child there is not worth a test. +TEST_CASE("InstanceLock yields to another process and reports it", "[InstanceLock]") +{ + ScopedTemporaryFile lock_file(".lock"); + const std::string path = lock_file.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); + char byte = ::flock(fd, LOCK_EX | LOCK_NB) == 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(); + } + // 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); + 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); + REQUIRE_FALSE(locked_during_cooldown); + REQUIRE(cooldown_wait < 4000ms); + // A guard inside the cool-down still takes the lock when it is free. + InstanceLock lock(path); + REQUIRE(lock.locked()); +} +#endif diff --git a/tests/libslic3r/test_utils.cpp b/tests/libslic3r/test_utils.cpp index 7880b783f1..ea6e36a9cb 100644 --- a/tests/libslic3r/test_utils.cpp +++ b/tests/libslic3r/test_utils.cpp @@ -8,8 +8,11 @@ #include #include +#include #include #include +#include +#include #ifndef _WIN32 #include // getuid @@ -62,6 +65,192 @@ 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"; + + 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)); +} + +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")); + boost::filesystem::permissions(target, boost::filesystem::owner_read | boost::filesystem::group_read | boost::filesystem::others_read); + + const std::error_code ec = write_file_atomically(target.string(), "replaced"); + boost::filesystem::permissions(target, boost::filesystem::owner_read | boost::filesystem::owner_write | boost::filesystem::group_read | boost::filesystem::others_read); + + REQUIRE(ec == std::errc::permission_denied); + std::string content; + load_string_file(target, content); + REQUIRE(content == "pinned"); +} + +TEST_CASE("write_file_atomically replaces a read-only target when asked to", "[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")); + boost::filesystem::permissions(target, boost::filesystem::owner_read | boost::filesystem::group_read | boost::filesystem::others_read); + + const std::error_code ec = write_file_atomically(target.string(), "replaced", { false, /*replace_read_only=*/true }); + boost::filesystem::permissions(target, boost::filesystem::owner_read | boost::filesystem::owner_write | boost::filesystem::group_read | boost::filesystem::others_read); + + REQUIRE_FALSE(ec); + std::string content; + load_string_file(target, content); + REQUIRE(content == "replaced"); +} + +TEST_CASE("write_file_atomically keeps bytes intact in binary mode", "[utils]") { + ScopedTemporaryDir dir; + const boost::filesystem::path target = dir.path() / "blob.bin"; + const std::string bytes("a\r\nb\0c", 6); + + REQUIRE_FALSE(write_file_atomically(target.string(), bytes, { /*binary=*/true })); + REQUIRE(boost::filesystem::file_size(target) == bytes.size()); +} + +#ifndef _WIN32 +TEST_CASE("write_file_atomically writes through a symlink and keeps the target's permissions", "[utils]") { + ScopedTemporaryDir dir; + const boost::filesystem::path real = dir.path() / "real.json"; + const boost::filesystem::path link = dir.path() / "link.json"; + REQUIRE_FALSE(write_file_atomically(real.string(), "first")); + boost::filesystem::permissions(real, boost::filesystem::owner_read | boost::filesystem::owner_write); + boost::filesystem::create_symlink(real, link); + + REQUIRE_FALSE(write_file_atomically(link.string(), "second")); + + REQUIRE(boost::filesystem::is_symlink(boost::filesystem::symlink_status(link))); + std::string content; + load_string_file(real, content); + REQUIRE(content == "second"); + + REQUIRE_FALSE(write_file_atomically(real.string(), "third")); + const auto perms = boost::filesystem::status(real).permissions() & boost::filesystem::all_all; + REQUIRE(perms == (boost::filesystem::owner_read | boost::filesystem::owner_write)); +} +#endif + +TEST_CASE("write_file_atomically survives two threads writing one target", "[utils]") { + ScopedTemporaryDir dir; + const boost::filesystem::path target = dir.path() / "shared.json"; + const std::string a(20000, 'a'), b(20000, 'b'); + + std::thread other([&] { + for (int i = 0; i < 50; ++i) + write_file_atomically(target.string(), a); + }); + for (int i = 0; i < 50; ++i) + write_file_atomically(target.string(), b); + other.join(); + + std::string content; + load_string_file(target, content); + const bool whole = content == a || content == b; + REQUIRE(whole); + // No temporary may be left; a scanner on Windows may briefly hold the old + // file under another name, so only the temporaries are counted. + size_t temporaries = 0; + for (auto &entry : boost::filesystem::directory_iterator(dir.path())) + if (entry.path().extension() == ".tmp") + ++temporaries; + REQUIRE(temporaries == 0); +} + +TEST_CASE("remove_stale_temp_files removes an old tree set aside for removal", "[utils]") { + ScopedTemporaryDir dir; + const boost::filesystem::path doomed = dir.path() / "removing.4242.0"; + boost::filesystem::create_directories(doomed / "sub"); + REQUIRE_FALSE(write_file_atomically((doomed / "sub" / "x.json").string(), "x")); + boost::filesystem::last_write_time(doomed, std::time(nullptr) - 7200); + boost::filesystem::create_directories(dir.path() / "removing.4242.1"); // just set aside: in progress + boost::filesystem::create_directories(dir.path() / "import.4242.0"); // an import in progress + + REQUIRE(remove_stale_temp_files(dir.path()) == 1); + REQUIRE_FALSE(boost::filesystem::exists(doomed)); + REQUIRE(boost::filesystem::exists(dir.path() / "removing.4242.1")); + REQUIRE(boost::filesystem::exists(dir.path() / "import.4242.0")); +} + +TEST_CASE("remove_if_stale_leftover judges one directory entry, as a scan does per file", "[utils]") { + ScopedTemporaryDir dir; + REQUIRE_FALSE(write_file_atomically((dir.path() / "a.json").string(), "{}")); + REQUIRE_FALSE(write_file_atomically((dir.path() / "a.json.123.7.tmp").string(), "{")); + REQUIRE_FALSE(write_file_atomically((dir.path() / "b.json.123.8.tmp").string(), "{")); + boost::filesystem::last_write_time(dir.path() / "a.json.123.7.tmp", std::time(nullptr) - 7200); + + REQUIRE_FALSE(remove_if_stale_leftover(boost::filesystem::directory_entry(dir.path() / "a.json"))); + REQUIRE(remove_if_stale_leftover(boost::filesystem::directory_entry(dir.path() / "a.json.123.7.tmp"))); + REQUIRE_FALSE(remove_if_stale_leftover(boost::filesystem::directory_entry(dir.path() / "b.json.123.8.tmp"))); // too young + REQUIRE(boost::filesystem::exists(dir.path() / "a.json")); + REQUIRE_FALSE(boost::filesystem::exists(dir.path() / "a.json.123.7.tmp")); + REQUIRE(boost::filesystem::exists(dir.path() / "b.json.123.8.tmp")); +} + +TEST_CASE("remove_stale_temp_files removes only old ...tmp files", "[utils]") { + ScopedTemporaryDir dir; + 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.5.old", "h.json.old" }) { + REQUIRE_FALSE(write_file_atomically((dir.path() / name).string(), "x")); + // Two hours old: well past the hour below which a temporary may still be in flight. + boost::filesystem::last_write_time(dir.path() / name, std::time(nullptr) - 7200); + } + // Just written: possibly another instance's in-flight save, so it stays. + REQUIRE_FALSE(write_file_atomically((dir.path() / "g.json.7.2.tmp").string(), "x")); + + SECTION("with a name prefix only matching names go; a numbered backup, a one-segment name or another suffix is not removed") { + 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.123.5.old")); + REQUIRE(boost::filesystem::exists(dir.path() / "a.json.99")); + 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()) == 2); + size_t entries = 0; + for (auto &entry : boost::filesystem::directory_iterator(dir.path())) { + (void) entry; + ++entries; + } + REQUIRE(entries == 10); + REQUIRE(boost::filesystem::exists(dir.path() / "a.json.123.5.old")); + 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")); + } +} + TEST_CASE("copy_file reports the OS error when the destination cannot be written", "[utils]") { ScopedTemporaryFile source(".txt"); { diff --git a/tests/libslic3r/test_vendor_cache.cpp b/tests/libslic3r/test_vendor_cache.cpp index 85e100f4d4..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,20 +1362,23 @@ 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")); const std::string before = slurp(cache); - // A directory where the temp file wants to go: the write cannot complete, - // and must not have destroyed what was already there to find that out. - const fs::path blocker = fs::path(cache + "." + std::to_string(get_current_pid()) + ".tmp"); - fs::create_directories(blocker); + // A read-only cache (the read-only attribute on Windows) is refused before + // anything is written, so what was there must survive the attempt. + fs::permissions(cache, fs::owner_read | fs::group_read | fs::others_read); REQUIRE_FALSE(save_one_vendor(cache, one_vendor("Durable"), "Durable", "2.0.0")); - CHECK(slurp(cache) == before); - fs::remove_all(blocker); + fs::permissions(cache, fs::owner_read | fs::owner_write | fs::group_read | fs::others_read); + CHECK(slurp(cache) == before); } TEST_CASE("a cache written by another build's option ordering still loads", "[VendorCache]")