From 18a4d70d4144446f962baf8492ab81af651702a6 Mon Sep 17 00:00:00 2001 From: Hanif Koh Date: Thu, 24 Sep 2026 17:26:46 +0800 Subject: [PATCH] Retry a Failed Lock File, Keep Special Targets in Place and Sweep Stale Temporaries A lock file that failed to open once stayed unopened for the rest of the process, so a scanner holding the fresh file for a moment on Windows silently disabled the lock for the session, and a lock call that throws, as it does on a share without a lock service, was re-attempted and logged by every guard. Both are now left alone for the cool-down and then retried. Renaming a fresh file over the target turned a symlinked preset into a plain file, reset its permissions to the umask default, and could not work at all for a settings export to a device or pipe, since a temporary cannot be created beside /dev/stdout. A target that is not a regular file is now written in place, an existing target keeps its permissions, and a directory that refuses the temporary but not the file falls back too. The inline PhysicalPrinter::save() overload was the one physical printer writer without the guard. load_info() took the lock redundantly under the scan guard and, in read-only mode, created user.lock per file; the bundle metadata reader now honours read_only too. The plugin config, the setup guide's profile cache and the vendor preset cache wrote through a temporary by hand; they use the helper, with a binary mode for the cache. A crash between temporary and rename left a ..tmp behind for good; AppConfig::load() and the preset scan remove stale ones while they hold the lock, and AppConfig::save() names its temporary the same way. --- src/libslic3r/AppConfig.cpp | 12 +++++- src/libslic3r/InstanceLock.cpp | 51 ++++++++++++++++--------- src/libslic3r/InstanceLock.hpp | 8 +++- src/libslic3r/Preset.cpp | 9 ++++- src/libslic3r/Preset.hpp | 2 +- src/libslic3r/PresetBundle.cpp | 8 ++-- src/libslic3r/PresetBundle.hpp | 3 +- src/libslic3r/PresetCacheFormat.cpp | 45 ++++++---------------- src/libslic3r/Utils.hpp | 10 ++++- src/libslic3r/utils.cpp | 50 ++++++++++++++++++++++--- src/slic3r/GUI/WebGuideDialog.cpp | 15 +------- src/slic3r/plugin/PluginConfig.cpp | 19 ++-------- tests/libslic3r/test_instance_lock.cpp | 29 ++++++++++++++ tests/libslic3r/test_utils.cpp | 52 ++++++++++++++++++++++++++ 14 files changed, 216 insertions(+), 97 deletions(-) diff --git a/src/libslic3r/AppConfig.cpp b/src/libslic3r/AppConfig.cpp index 09eacac87d..0877aa3f1e 100644 --- a/src/libslic3r/AppConfig.cpp +++ b/src/libslic3r/AppConfig.cpp @@ -737,6 +737,10 @@ std::string AppConfig::load() // Keep another instance from replacing or restoring the file mid-read. InstanceLock instance_lock(lock_path()); + if (instance_lock.locked()) { + const boost::filesystem::path conf(config_path()); + remove_stale_temp_files(conf.parent_path(), conf.filename().string()); + } // 1) Read the complete config file into a boost::property_tree. namespace pt = boost::property_tree; @@ -988,7 +992,7 @@ void AppConfig::save() // to avoid race conditions with multiple instances of Slic3r const auto path = config_path(); InstanceLock instance_lock(lock_path()); - std::string path_pid = (boost::format("%1%.%2%") % path % get_current_pid()).str(); + std::string path_pid = (boost::format("%1%.%2%.tmp") % path % get_current_pid()).str(); json j; @@ -1158,6 +1162,10 @@ std::string AppConfig::load() { // Keep another instance from replacing or restoring the file mid-read. InstanceLock instance_lock(lock_path()); + if (instance_lock.locked()) { + const boost::filesystem::path conf(config_path()); + remove_stale_temp_files(conf.parent_path(), conf.filename().string()); + } // 1) Read the complete config file into a boost::property_tree. namespace pt = boost::property_tree; @@ -1297,7 +1305,7 @@ void AppConfig::save() // to avoid race conditions with multiple instances of Slic3r const auto path = config_path(); InstanceLock instance_lock(lock_path()); - std::string path_pid = (boost::format("%1%.%2%") % path % get_current_pid()).str(); + std::string path_pid = (boost::format("%1%.%2%.tmp") % path % get_current_pid()).str(); std::stringstream config_ss; if (m_mode == EAppMode::Editor) diff --git a/src/libslic3r/InstanceLock.cpp b/src/libslic3r/InstanceLock.cpp index c6bda14f9d..bb8f825038 100644 --- a/src/libslic3r/InstanceLock.cpp +++ b/src/libslic3r/InstanceLock.cpp @@ -21,12 +21,15 @@ namespace Slic3r { struct InstanceLock::Slot { std::recursive_mutex mutex; - // Absent when the lock file could not be created or opened. + // Absent until the lock file has been created and opened. std::unique_ptr file_lock; int depth{0}; bool file_locked{false}; // After a timed-out wait, guards skip waiting until this point. std::chrono::steady_clock::time_point skip_waiting_until{}; + // After a failed open or a failing lock call, guards skip the file lock + // entirely until this point. + std::chrono::steady_clock::time_point retry_at{}; }; InstanceLock::Slot &InstanceLock::slot_for(const std::string &lock_file_path) @@ -38,32 +41,42 @@ InstanceLock::Slot &InstanceLock::slot_for(const std::string &lock_file_path) std::lock_guard guard(registry_mutex); std::unique_ptr &slot = (*registry)[lock_file_path]; - if (! slot) { + if (! slot) slot = std::make_unique(); - try { - // file_lock only opens existing files. - boost::nowide::ofstream(lock_file_path, std::ios::app).close(); -#ifdef _WIN32 - slot->file_lock = std::make_unique(boost::nowide::widen(lock_file_path).c_str()); -#else - slot->file_lock = std::make_unique(lock_file_path.c_str()); -#endif - } catch (const std::exception &e) { - BOOST_LOG_TRIVIAL(warning) << "Cannot open lock file " << lock_file_path << ": " << e.what() - << "; other instances are not excluded from writing"; - } - } return *slot; } +// Creates the lock file if needed and opens it. Called with the slot mutex held. +static bool open_lock_file(std::unique_ptr &file_lock, const std::string &lock_file_path) +{ + try { + // file_lock only opens existing files. + boost::nowide::ofstream(lock_file_path, std::ios::app).close(); +#ifdef _WIN32 + file_lock = std::make_unique(boost::nowide::widen(lock_file_path).c_str()); +#else + file_lock = std::make_unique(lock_file_path.c_str()); +#endif + return true; + } catch (const std::exception &e) { + BOOST_LOG_TRIVIAL(warning) << "Cannot open lock file " << lock_file_path << ": " << e.what() + << "; other instances are not excluded from writing"; + return 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->mutex.lock(); - if (m_slot->depth++ == 0 && m_slot->file_lock) { - const auto now = std::chrono::steady_clock::now(); + const auto now = std::chrono::steady_clock::now(); + if (m_slot->depth++ == 0 && now >= m_slot->retry_at) { + if (! m_slot->file_lock && ! open_lock_file(m_slot->file_lock, lock_file_path)) + m_slot->retry_at = now + cooldown; + } + if (m_slot->depth == 1 && m_slot->file_lock && now >= m_slot->retry_at) { const bool wait = now >= m_slot->skip_waiting_until; const auto deadline = now + timeout; for (;;) { @@ -73,7 +86,9 @@ InstanceLock::InstanceLock(const std::string &lock_file_path, std::chrono::milli break; } } catch (const std::exception &e) { - BOOST_LOG_TRIVIAL(warning) << "Cannot lock " << lock_file_path << ": " << e.what(); + m_slot->retry_at = std::chrono::steady_clock::now() + cooldown; + BOOST_LOG_TRIVIAL(warning) << "Cannot lock " << lock_file_path << ": " << e.what() + << "; proceeding without the lock for the next " << cooldown.count() << " ms"; break; } if (! wait) diff --git a/src/libslic3r/InstanceLock.hpp b/src/libslic3r/InstanceLock.hpp index 8adc7fc42d..3c76bcaa28 100644 --- a/src/libslic3r/InstanceLock.hpp +++ b/src/libslic3r/InstanceLock.hpp @@ -19,7 +19,9 @@ namespace Slic3r { // mutex and locked() reports false. Writes then proceed unprotected rather than // letting one hung instance block every other one from saving. After such a // timeout the same lock file is not waited on again for `cooldown`, so a batch -// of saves pays the wait once rather than once per file. +// of saves pays the wait once rather than once per file; a lock file that could +// not be opened, or a lock call that fails outright (a share without a lock +// service), is likewise retried only after `cooldown`. // // 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. @@ -30,7 +32,9 @@ class InstanceLock { public: static constexpr std::chrono::milliseconds default_timeout{2000}; - static constexpr std::chrono::milliseconds cooldown{10000}; + // How long a lock file is left alone after a timed-out wait, a failed open + // or a failing lock call. 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); diff --git a/src/libslic3r/Preset.cpp b/src/libslic3r/Preset.cpp index 93d141da05..4b72801f75 100644 --- a/src/libslic3r/Preset.cpp +++ b/src/libslic3r/Preset.cpp @@ -611,7 +611,6 @@ Preset::Type Preset::get_type_from_string(std::string type_str) void Preset::load_info(const std::string& file) { - InstanceLock instance_lock(user_presets_lock_path()); try { boost::property_tree::ptree tree; boost::nowide::ifstream ifs(file); @@ -1722,6 +1721,8 @@ void PresetCollection::load_presets( // A read-only scan (the CLI) never rewrites or deletes, and many CLI jobs // may share one data dir, so it does not queue behind the others. InstanceLock instance_lock(read_only ? std::string() : user_presets_lock_path()); + if (instance_lock.locked()) + remove_stale_temp_files(dir); //BBS: change to json format for (auto &dir_entry : boost::filesystem::directory_iterator(dir)) { @@ -4219,6 +4220,12 @@ 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()); diff --git a/src/libslic3r/Preset.hpp b/src/libslic3r/Preset.hpp index aef97021b7..202e676f66 100644 --- a/src/libslic3r/Preset.hpp +++ b/src/libslic3r/Preset.hpp @@ -1057,7 +1057,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 4dac4d3a49..c73fa27b08 100644 --- a/src/libslic3r/PresetBundle.cpp +++ b/src/libslic3r/PresetBundle.cpp @@ -1241,7 +1241,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 (!metadata.load_from_json(metadata_file.string(), read_only)) continue; metadata.print_presets.clear(); metadata.filament_presets.clear(); metadata.printer_presets.clear(); @@ -1276,7 +1276,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 (!metadata.load_from_json(metadata_file.string(), read_only)) continue; metadata.print_presets.clear(); metadata.filament_presets.clear(); metadata.printer_presets.clear(); @@ -7867,9 +7867,9 @@ bool PresetBundle::check_duplicate_filament_subtypes() const } // Orca: BundleMetadata method implementations -bool BundleMetadata::load_from_json(const std::string& path) +bool BundleMetadata::load_from_json(const std::string& path, bool read_only) { - InstanceLock instance_lock(user_presets_lock_path()); + InstanceLock instance_lock(read_only ? std::string() : user_presets_lock_path()); try { boost::nowide::ifstream ifs(path); if (!ifs.good()) diff --git a/src/libslic3r/PresetBundle.hpp b/src/libslic3r/PresetBundle.hpp index 5a8f34e706..54b3b97777 100644 --- a/src/libslic3r/PresetBundle.hpp +++ b/src/libslic3r/PresetBundle.hpp @@ -129,7 +129,8 @@ struct BundleMetadata bool not_found{false}; bool unauthorized{false}; - bool load_from_json(const std::string& path); + // A read-only load (the CLI) takes no instance lock. + bool load_from_json(const std::string& path, bool read_only = false); bool save_to_json(const std::string& path) const; }; diff --git a/src/libslic3r/PresetCacheFormat.cpp b/src/libslic3r/PresetCacheFormat.cpp index accebba7b4..0451e8d9be 100644 --- a/src/libslic3r/PresetCacheFormat.cpp +++ b/src/libslic3r/PresetCacheFormat.cpp @@ -400,46 +400,25 @@ 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(); + std::string payload(reinterpret_cast(&fhdr), sizeof(fhdr)); + payload += blob; + if (const std::error_code ec = write_file_atomically(path, payload, /*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 64950a64f5..042ebd1087 100644 --- a/src/libslic3r/Utils.hpp +++ b/src/libslic3r/Utils.hpp @@ -226,8 +226,14 @@ extern std::vector split_string(const std::string &str, char delimi extern std::error_code rename_file(const std::string &from, const std::string &to); // Write `content` to `path` through a temporary file beside it that is then renamed // over the target, so a concurrent reader sees the old or the new file, never a -// partial one. The temporary file is removed on failure. -extern std::error_code write_file_atomically(const std::string &path, const std::string &content); +// partial one. The temporary file 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. +extern std::error_code write_file_atomically(const std::string &path, const std::string &content, bool binary = false); +// Remove the `..tmp` files a crashed write_file_atomically() left in +// `dir`, restricted to names starting with `name_prefix` when given. Call it only +// while holding the lock that guards writes into `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 654c4962e1..03f78de70d 100644 --- a/src/libslic3r/utils.cpp +++ b/src/libslic3r/utils.cpp @@ -711,10 +711,10 @@ std::error_code rename_file(const std::string &from, const std::string &to) #endif } -static std::error_code write_whole_file(const std::string &path, const std::string &content) +static std::error_code write_whole_file(const std::string &path, const std::string &content, bool binary) { errno = 0; - boost::nowide::ofstream out(path, std::ios::out | std::ios::trunc); + boost::nowide::ofstream out(path, std::ios::out | std::ios::trunc | (binary ? std::ios::binary : std::ios::openmode{})); out << content; out.close(); if (! out.fail()) @@ -722,13 +722,24 @@ static std::error_code write_whole_file(const std::string &path, const std::stri return std::make_error_code(errno != 0 ? static_cast(errno) : std::errc::io_error); } -std::error_code write_file_atomically(const std::string &path, const std::string &content) +std::error_code write_file_atomically(const std::string &path, const std::string &content, bool 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_regular_file(target)) + return write_whole_file(path, content, binary); + const std::string tmp_path = path + "." + std::to_string(get_current_pid()) + ".tmp"; - if (std::error_code ec = write_whole_file(tmp_path, content)) { + if (std::error_code ec = write_whole_file(tmp_path, content, binary)) { boost::nowide::remove(tmp_path.c_str()); + // A directory that allows writing the file but not creating one beside it. + if (target_exists && ec == std::errc::permission_denied) + return write_whole_file(path, content, binary); return ec; } + if (target_exists) + boost::filesystem::permissions(tmp_path, target.permissions(), bec); std::error_code ec = rename_file(tmp_path, path); if (ec) boost::nowide::remove(tmp_path.c_str()); @@ -739,12 +750,41 @@ std::error_code write_file_atomically(const std::string &path, const std::string // in place the way this used to work before the atomic path existed. if (ec == std::errc::permission_denied) { BOOST_LOG_TRIVIAL(warning) << "Cannot replace " << path << " while another process holds it open; writing in place"; - ec = write_whole_file(path, content); + ec = write_whole_file(path, content, binary); } #endif return ec; } +size_t remove_stale_temp_files(const boost::filesystem::path &dir, const std::string &name_prefix) +{ + // ..tmp + auto is_temp_name = [&name_prefix](const std::string &name) { + if (name.compare(0, name_prefix.size(), name_prefix) != 0) + return false; + static const std::string suffix = ".tmp"; + if (name.size() <= suffix.size() || name.compare(name.size() - suffix.size(), suffix.size(), suffix) != 0) + return false; + const size_t digits_end = name.size() - suffix.size(); + const size_t dot = name.rfind('.', digits_end - 1); + if (dot == std::string::npos || dot == 0 || dot + 1 == digits_end) + return false; + return std::all_of(name.begin() + dot + 1, name.begin() + digits_end, [](char c) { return c >= '0' && c <= '9'; }); + }; + 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 (! boost::filesystem::is_regular_file(it->symlink_status()) || ! is_temp_name(it->path().filename().string())) + continue; + if (boost::filesystem::remove(it->path(), ec)) { + BOOST_LOG_TRIVIAL(info) << "Removed stale temporary file " << it->path(); + ++ removed; + } + ec.clear(); + } + 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/WebGuideDialog.cpp b/src/slic3r/GUI/WebGuideDialog.cpp index 78030c68da..9ceb2012f8 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/plugin/PluginConfig.cpp b/src/slic3r/plugin/PluginConfig.cpp index a6f0ead18b..14d24fabe1 100644 --- a/src/slic3r/plugin/PluginConfig.cpp +++ b/src/slic3r/plugin/PluginConfig.cpp @@ -249,21 +249,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/test_instance_lock.cpp b/tests/libslic3r/test_instance_lock.cpp index e72d5b8798..ab49458d39 100644 --- a/tests/libslic3r/test_instance_lock.cpp +++ b/tests/libslic3r/test_instance_lock.cpp @@ -62,6 +62,35 @@ TEST_CASE("InstanceLock is a no-op for an empty path and survives an unwritable 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(); + const auto saved_cooldown = InstanceLock::cooldown; + InstanceLock::cooldown = 50ms; + + bool before_dir, during_cooldown, after_cooldown; + { + InstanceLock lock(path, 100ms); + before_dir = lock.locked(); + } + boost::filesystem::create_directories(dir.path() / "later"); + { + InstanceLock lock(path, 100ms); + during_cooldown = lock.locked(); + } + std::this_thread::sleep_for(80ms); + { + InstanceLock lock(path, 100ms); + after_cooldown = lock.locked(); + } + InstanceLock::cooldown = saved_cooldown; + + REQUIRE_FALSE(before_dir); + REQUIRE_FALSE(during_cooldown); + REQUIRE(after_cooldown); +} + TEST_CASE("InstanceLock serialises the threads of one process", "[InstanceLock]") { ScopedTemporaryFile lock_file(".lock"); diff --git a/tests/libslic3r/test_utils.cpp b/tests/libslic3r/test_utils.cpp index cac88d30b4..6e2cfa1bbf 100644 --- a/tests/libslic3r/test_utils.cpp +++ b/tests/libslic3r/test_utils.cpp @@ -90,6 +90,58 @@ TEST_CASE("write_file_atomically reports a missing directory and writes nothing" REQUIRE_FALSE(boost::filesystem::exists(target)); } +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("remove_stale_temp_files removes only ..tmp files", "[utils]") { + ScopedTemporaryDir dir; + for (const char *name : { "a.json.123.tmp", "b.info.4.tmp", "c.json", "d.tmp", "e.json.x.tmp", "f.json..tmp" }) + REQUIRE_FALSE(write_file_atomically((dir.path() / name).string(), "x")); + + SECTION("with a name prefix only matching names go") { + REQUIRE(remove_stale_temp_files(dir.path(), "a.json") == 1); + REQUIRE_FALSE(boost::filesystem::exists(dir.path() / "a.json.123.tmp")); + REQUIRE(boost::filesystem::exists(dir.path() / "b.info.4.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 == 4); + } +} + TEST_CASE("copy_file reports the OS error when the destination cannot be written", "[utils]") { ScopedTemporaryFile source(".txt"); {