diff --git a/src/libslic3r/InstanceLock.cpp b/src/libslic3r/InstanceLock.cpp index bb8f825038..450ef37b47 100644 --- a/src/libslic3r/InstanceLock.cpp +++ b/src/libslic3r/InstanceLock.cpp @@ -34,12 +34,12 @@ struct InstanceLock::Slot InstanceLock::Slot &InstanceLock::slot_for(const std::string &lock_file_path) { - static std::mutex registry_mutex; // Never freed: the slots keep the lock files open for as long as anything // in the process may still save, including during static destruction. - static auto *registry = new std::map>(); + static auto *registry_mutex = new std::mutex(); + static auto *registry = new std::map>(); - std::lock_guard guard(registry_mutex); + std::lock_guard guard(*registry_mutex); std::unique_ptr &slot = (*registry)[lock_file_path]; if (! slot) slot = std::make_unique(); diff --git a/src/libslic3r/Preset.cpp b/src/libslic3r/Preset.cpp index 4b72801f75..ea0678dc9d 100644 --- a/src/libslic3r/Preset.cpp +++ b/src/libslic3r/Preset.cpp @@ -713,13 +713,16 @@ void Preset::save(DynamicPrintConfig* parent_config) else from_str = std::string("Default"); - InstanceLock instance_lock(user_presets_lock_path()); - boost::filesystem::create_directories(fs::path(this->file).parent_path()); const std::string bare_name = get_preset_bare_name(this->name); + // 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; @@ -755,14 +758,16 @@ 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; } + + InstanceLock instance_lock(user_presets_lock_path()); + boost::filesystem::create_directories(fs::path(this->file).parent_path()); + to_save->save_to_json(this->file, bare_name, from_str, this->version.to_string()); BOOST_LOG_TRIVIAL(info) << __FUNCTION__ << " save config for: " << this->name << " and filament_id: " << filament_id << " and base_id: " << this->base_id; // Bundle presets are synced via bundle_id and don't need individual .info files. @@ -4345,7 +4350,6 @@ void PhysicalPrinterCollection::load_printers( std::string errors_cummulative; // Store the loaded printers into a new vector, otherwise the binary search for already existing presets would be broken. std::deque printers_loaded; - InstanceLock instance_lock(user_presets_lock_path()); //BBS: change to json format for (auto& dir_entry : boost::filesystem::directory_iterator(dir)) { @@ -4553,7 +4557,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; } @@ -4565,7 +4572,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/PresetBundle.cpp b/src/libslic3r/PresetBundle.cpp index c73fa27b08..1bd2de3fc2 100644 --- a/src/libslic3r/PresetBundle.cpp +++ b/src/libslic3r/PresetBundle.cpp @@ -1232,6 +1232,8 @@ PresetsConfigSubstitutions PresetBundle::load_user_presets(std::string user, For fs::path local_dir(folder / PRESET_LOCAL_DIR); if (fs::exists(local_dir)) { dir_user_presets_local = local_dir; + // Held across the metadata reads; a read-only load (the CLI) takes no lock. + InstanceLock instance_lock(read_only ? std::string() : user_presets_lock_path()); for (auto& entry : fs::directory_iterator(local_dir)) { if (!fs::is_directory(entry.path())) continue; @@ -1241,7 +1243,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(), read_only)) continue; + if (!metadata.load_from_json(metadata_file.string())) continue; metadata.print_presets.clear(); metadata.filament_presets.clear(); metadata.printer_presets.clear(); @@ -1267,6 +1269,7 @@ PresetsConfigSubstitutions PresetBundle::load_user_presets(std::string user, For // Load bundle metadata from _subscribed directory fs::path subscribed_dir(folder / PRESET_SUBSCRIBED_DIR); if (fs::exists(subscribed_dir)) { + InstanceLock instance_lock(read_only ? std::string() : user_presets_lock_path()); for (auto& entry : fs::directory_iterator(subscribed_dir)) { if (!fs::is_directory(entry.path())) continue; @@ -1276,7 +1279,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(), read_only)) continue; + if (!metadata.load_from_json(metadata_file.string())) continue; metadata.print_presets.clear(); metadata.filament_presets.clear(); metadata.printer_presets.clear(); @@ -7867,9 +7870,8 @@ bool PresetBundle::check_duplicate_filament_subtypes() const } // Orca: BundleMetadata method implementations -bool BundleMetadata::load_from_json(const std::string& path, bool read_only) +bool BundleMetadata::load_from_json(const std::string& 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 54b3b97777..5a8f34e706 100644 --- a/src/libslic3r/PresetBundle.hpp +++ b/src/libslic3r/PresetBundle.hpp @@ -129,8 +129,7 @@ struct BundleMetadata bool not_found{false}; bool unauthorized{false}; - // A read-only load (the CLI) takes no instance lock. - bool load_from_json(const std::string& path, bool read_only = false); + bool load_from_json(const std::string& path); bool save_to_json(const std::string& path) const; }; diff --git a/src/libslic3r/PresetCacheFormat.cpp b/src/libslic3r/PresetCacheFormat.cpp index 0451e8d9be..9b8f65f74e 100644 --- a/src/libslic3r/PresetCacheFormat.cpp +++ b/src/libslic3r/PresetCacheFormat.cpp @@ -410,7 +410,9 @@ bool write_cache_blob(const std::string& path, const std::string& blob) fhdr.version = CACHE_VERSION; fhdr.data_size = static_cast(blob.size()); fhdr.crc32 = crc.checksum(); - std::string payload(reinterpret_cast(&fhdr), sizeof(fhdr)); + std::string payload; + payload.reserve(sizeof(fhdr) + blob.size()); + payload.append(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(); diff --git a/src/libslic3r/Utils.hpp b/src/libslic3r/Utils.hpp index 042ebd1087..0d8115ef02 100644 --- a/src/libslic3r/Utils.hpp +++ b/src/libslic3r/Utils.hpp @@ -231,8 +231,9 @@ extern std::error_code rename_file(const std::string &from, const std::string &t // 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. +// `dir` at least ten minutes ago, 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 { diff --git a/src/libslic3r/utils.cpp b/src/libslic3r/utils.cpp index 03f78de70d..32847c07b8 100644 --- a/src/libslic3r/utils.cpp +++ b/src/libslic3r/utils.cpp @@ -707,7 +707,15 @@ std::error_code rename_file(const std::string &from, const std::string &to) #else // rename(2) replaces an existing target atomically; removing it first would // leave a window in which the file does not exist at all. - return std::make_error_code(static_cast(boost::nowide::rename(from.c_str(), to.c_str()) == 0 ? 0 : errno)); + if (boost::nowide::rename(from.c_str(), to.c_str()) == 0) + return {}; + const int err = errno; + // Some mounts (sshfs without its rename workaround, for one) refuse to + // replace an existing target in one step; take the old two-step route there. + if ((err == EPERM || err == EEXIST || err == ENOTEMPTY) && 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 } @@ -741,18 +749,16 @@ std::error_code write_file_atomically(const std::string &path, const std::string if (target_exists) boost::filesystem::permissions(tmp_path, target.permissions(), bec); std::error_code ec = rename_file(tmp_path, path); - if (ec) + if (ec) { boost::nowide::remove(tmp_path.c_str()); -#ifdef _WIN32 - // Windows refuses to replace a file another process holds open without - // FILE_SHARE_DELETE, which is how the C runtime opens files for reading. - // Losing the save is worse than a reader seeing a partial file, so write - // in place the way this used to work before the atomic path existed. - if (ec == std::errc::permission_denied) { - BOOST_LOG_TRIVIAL(warning) << "Cannot replace " << path << " while another process holds it open; writing in place"; + // Windows refuses to replace a file another process holds open without + // FILE_SHARE_DELETE, which is how the C runtime opens files for reading, + // and some mounts cannot replace a file in one step 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"; ec = write_whole_file(path, content, binary); } -#endif return ec; } @@ -771,11 +777,20 @@ size_t remove_stale_temp_files(const boost::filesystem::path &dir, const std::st return false; return std::all_of(name.begin() + dot + 1, name.begin() + digits_end, [](char c) { return c >= '0' && c <= '9'; }); }; + // An instance that gave up waiting for the lock writes unlocked by design, + // so a temporary this young may still be in flight; a crash leftover is old. + constexpr std::time_t stale_age = 10 * 60; + const std::time_t now = std::time(nullptr); 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())) + if (! boost::filesystem::is_regular_file(it->symlink_status(ec)) || ! is_temp_name(it->path().filename().string())) continue; + const std::time_t written = boost::filesystem::last_write_time(it->path(), ec); + if (ec || now - written < stale_age) { + ec.clear(); + continue; + } if (boost::filesystem::remove(it->path(), ec)) { BOOST_LOG_TRIVIAL(info) << "Removed stale temporary file " << it->path(); ++ removed; diff --git a/tests/libslic3r/test_instance_lock.cpp b/tests/libslic3r/test_instance_lock.cpp index ab49458d39..6c7ac43fb8 100644 --- a/tests/libslic3r/test_instance_lock.cpp +++ b/tests/libslic3r/test_instance_lock.cpp @@ -67,9 +67,10 @@ TEST_CASE("InstanceLock retries a lock file it could not open once the cool-down ScopedTemporaryDir dir; const std::string path = (dir.path() / "later" / "shared.lock").string(); const auto saved_cooldown = InstanceLock::cooldown; - InstanceLock::cooldown = 50ms; + 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(); @@ -79,7 +80,8 @@ TEST_CASE("InstanceLock retries a lock file it could not open once the cool-down InstanceLock lock(path, 100ms); during_cooldown = lock.locked(); } - std::this_thread::sleep_for(80ms); + 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(); @@ -87,7 +89,10 @@ TEST_CASE("InstanceLock retries a lock file it could not open once the cool-down InstanceLock::cooldown = saved_cooldown; REQUIRE_FALSE(before_dir); - REQUIRE_FALSE(during_cooldown); + // 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); } diff --git a/tests/libslic3r/test_utils.cpp b/tests/libslic3r/test_utils.cpp index 6e2cfa1bbf..f1ed2b2f59 100644 --- a/tests/libslic3r/test_utils.cpp +++ b/tests/libslic3r/test_utils.cpp @@ -8,6 +8,7 @@ #include #include +#include #include #include #include @@ -121,10 +122,15 @@ TEST_CASE("write_file_atomically writes through a symlink and keeps the target's } #endif -TEST_CASE("remove_stale_temp_files removes only ..tmp files", "[utils]") { +TEST_CASE("remove_stale_temp_files removes only old ..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" }) + 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")); + // An hour old: long past the age below which a temporary may still be in flight. + boost::filesystem::last_write_time(dir.path() / name, std::time(nullptr) - 3600); + } + // Just written: possibly another instance's in-flight save, so it stays. + REQUIRE_FALSE(write_file_atomically((dir.path() / "g.json.7.tmp").string(), "x")); SECTION("with a name prefix only matching names go") { REQUIRE(remove_stale_temp_files(dir.path(), "a.json") == 1); @@ -138,7 +144,8 @@ TEST_CASE("remove_stale_temp_files removes only ..tmp files", "[utils (void) entry; ++entries; } - REQUIRE(entries == 4); + REQUIRE(entries == 5); + REQUIRE(boost::filesystem::exists(dir.path() / "g.json.7.tmp")); } }