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]")