diff --git a/src/libslic3r/InstanceLock.cpp b/src/libslic3r/InstanceLock.cpp index 13d6cc950e..2cfe7ac8f9 100644 --- a/src/libslic3r/InstanceLock.cpp +++ b/src/libslic3r/InstanceLock.cpp @@ -11,6 +11,8 @@ #include #endif +#include "Utils.hpp" + namespace Slic3r { // One slot per lock file, shared by every guard in the process. A POSIX fcntl @@ -22,6 +24,9 @@ struct InstanceLock::Slot std::recursive_mutex mutex; // Absent until the lock file has been created and opened. std::unique_ptr file_lock; + // What the lock file was when it was opened; a different answer means the + // path was unlinked or replaced and the open handle locks a dead file. + std::string identity; int depth{0}; bool file_locked{false}; // After a timed-out wait, guards skip waiting until this point. @@ -57,6 +62,7 @@ bool InstanceLock::open_lock_file(Slot &slot, const std::string &lock_file_path) #else slot.file_lock = std::make_unique(lock_file_path.c_str()); #endif + slot.identity = file_identity(lock_file_path); return true; } catch (const std::exception &e) { slot.retry_at = std::chrono::steady_clock::now() + cooldown; @@ -73,6 +79,12 @@ InstanceLock::InstanceLock(const std::string &lock_file_path, std::chrono::milli m_slot = &slot_for(lock_file_path); m_slot_guard = std::unique_lock(m_slot->mutex); const auto now = std::chrono::steady_clock::now(); + // A handle to a lock file that was deleted or recreated since it was opened + // would lock a file no other instance can see. + if (m_slot->depth == 0 && m_slot->file_lock && file_identity(lock_file_path) != m_slot->identity) { + BOOST_LOG_TRIVIAL(info) << "Lock file " << lock_file_path << " was replaced; reopening it"; + m_slot->file_lock.reset(); + } if (m_slot->depth == 0 && now >= m_slot->retry_at && (m_slot->file_lock || open_lock_file(*m_slot, lock_file_path))) { const bool wait = now >= m_slot->skip_waiting_until; diff --git a/src/libslic3r/Preset.cpp b/src/libslic3r/Preset.cpp index 23084eecd6..d96aa4a766 100644 --- a/src/libslic3r/Preset.cpp +++ b/src/libslic3r/Preset.cpp @@ -765,10 +765,15 @@ void Preset::save(DynamicPrintConfig* parent_config) 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()); - 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; + if (const std::error_code ec = write_file_atomically(this->file, json.str())) + BOOST_LOG_TRIVIAL(error) << __FUNCTION__ << ": failed to save " << this->file << ": " << ec.message(); + else + 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. if (! this->is_from_bundle()) { @@ -1722,10 +1727,11 @@ void PresetCollection::load_presets( std::set *key_set1 = nullptr, *key_set2 = nullptr; Preset::get_extruder_names_and_keysets(m_type, extruder_id_name, extruder_variant_name, &key_set1, &key_set2); - // Held across the scan so no instance replaces or removes a file mid-read. - InstanceLock instance_lock(user_presets_lock_path(read_only)); - if (instance_lock.locked()) - remove_stale_temp_files(dir); + { + InstanceLock instance_lock(user_presets_lock_path(read_only)); + if (instance_lock.locked()) + remove_stale_temp_files(dir); + } //BBS: change to json format for (auto &dir_entry : boost::filesystem::directory_iterator(dir)) { @@ -1747,30 +1753,36 @@ void PresetCollection::load_presets( preset.file = dir_entry.path().string(); // Load the preset file, apply preset values on top of defaults. try { - 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); + ConfigSubstitutions config_substitutions; + { + // Per file, so no instance replaces or removes it mid-read, and a + // save on another thread never waits for the whole scan. + InstanceLock instance_lock(user_presets_lock_path(read_only)); + fs::path idx_path(preset.file); + idx_path.replace_extension(".info"); + if (fs::exists(idx_path)) { + preset.load_info(idx_path.string()); + } + //BBS: change to json format + //ConfigSubstitutions config_substitutions = config.load_from_ini(preset.file, substitution_rule); + 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); + 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) }); - 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); - BOOST_LOG_TRIVIAL(error) << boost::format("parse config %1% failed")%preset.file; - ++m_errors; - continue; - } std::string version_str = key_values[BBL_JSON_KEY_VERSION]; boost::optional version = Semver::parse(version_str); diff --git a/src/libslic3r/Utils.hpp b/src/libslic3r/Utils.hpp index 9de742dc14..380190ac64 100644 --- a/src/libslic3r/Utils.hpp +++ b/src/libslic3r/Utils.hpp @@ -235,9 +235,12 @@ extern std::error_code write_file_atomically(const std::string &path, std::initi inline std::error_code write_file_atomically(const std::string &path, const std::string &content, bool binary = false) { return write_file_atomically(path, { std::string_view(content) }, binary); } // Remove the `...tmp` files a crashed write_file_atomically() left in -// `dir` at least ten minutes ago, only names starting with `name_prefix` when it is +// `dir` at least an hour ago, only names starting with `name_prefix` when it is // given. Meant for directories the application owns. Returns how many were removed. extern size_t remove_stale_temp_files(const boost::filesystem::path &dir, const std::string &name_prefix = std::string()); +// Names the file object behind `path` (device and inode, or volume and file index), +// for noticing that a path was unlinked and recreated. Empty when it cannot be read. +extern std::string file_identity(const std::string &path); enum CopyFileResult { SUCCESS = 0, diff --git a/src/libslic3r/utils.cpp b/src/libslic3r/utils.cpp index 5f3cf6680c..baf04117a9 100644 --- a/src/libslic3r/utils.cpp +++ b/src/libslic3r/utils.cpp @@ -10,6 +10,8 @@ #include #include #include +#include +#include #include #include #include @@ -767,50 +769,84 @@ std::error_code write_file_atomically(const std::string &path, std::initializer_ 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. - if (target_exists && ec == std::errc::permission_denied) + if (target_exists && ec == std::errc::permission_denied) { + BOOST_LOG_TRIVIAL(warning) << "Cannot create a temporary beside " << path << " (" << ec.message() << "); writing in place"; return write_whole_file(path, chunks, binary); + } return ec; } #ifndef _WIN32 - // On Windows a read-only bit on the temporary would stop the rename itself. + // 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 - std::error_code ec = rename_file(tmp_path, path); + // Windows refuses to replace a file another process holds open without + // FILE_SHARE_DELETE, which is how the C runtime opens files for reading; a + // reader is done in milliseconds, so wait it out before giving up on the + // atomic path. + std::error_code ec; + for (int attempt = 0; attempt < 20; ++ attempt) { + ec = rename_file(tmp_path, path); + if (ec != std::errc::permission_denied) + break; + std::this_thread::sleep_for(std::chrono::milliseconds(25)); + } if (ec) { boost::nowide::remove(tmp_path.c_str()); - // 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. + // Still refused, or a mount that 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"; return write_whole_file(path, chunks, binary); } -#ifdef _WIN32 - if (target_exists) - boost::filesystem::permissions(path, target.permissions(), bec); -#endif return {}; } +std::string file_identity(const std::string &path) +{ +#ifdef _WIN32 + const std::wstring wide = boost::nowide::widen(path); + HANDLE handle = ::CreateFileW(wide.c_str(), 0, FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, nullptr, OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, nullptr); + if (handle == INVALID_HANDLE_VALUE) + return {}; + BY_HANDLE_FILE_INFORMATION info; + const bool ok = ::GetFileInformationByHandle(handle, &info) != 0; + ::CloseHandle(handle); + if (! ok) + return {}; + return std::to_string(info.dwVolumeSerialNumber) + ":" + std::to_string((static_cast(info.nFileIndexHigh) << 32) | info.nFileIndexLow); +#else + struct stat st; + if (::stat(path.c_str(), &st) != 0) + return {}; + return std::to_string(static_cast(st.st_dev)) + ":" + std::to_string(static_cast(st.st_ino)); +#endif +} + size_t remove_stale_temp_files(const boost::filesystem::path &dir, const std::string &name_prefix) { - // ....tmp + // .....tmp, exactly the shape write_file_atomically() makes. 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); - return dot != std::string::npos && dot != 0 && dot + 1 != digits_end && - std::all_of(name.begin() + dot + 1, name.begin() + digits_end, [](char c) { return c >= '0' && c <= '9'; }); + size_t end = name.size() - suffix.size(); + for (int segment = 0; segment < 2; ++ 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; }; // 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; + // 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 now = std::time(nullptr); size_t removed = 0; boost::system::error_code ec; diff --git a/tests/libslic3r/test_instance_lock.cpp b/tests/libslic3r/test_instance_lock.cpp index 6c7ac43fb8..6ff9cca250 100644 --- a/tests/libslic3r/test_instance_lock.cpp +++ b/tests/libslic3r/test_instance_lock.cpp @@ -96,6 +96,24 @@ TEST_CASE("InstanceLock retries a lock file it could not open once the cool-down 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()); + // Only a reopen recreates the file; a guard still holding the unlinked one + // would leave the path missing. (The inode number itself may be reused once + // the old handle is closed, so it is no proof either way.) + REQUIRE(boost::filesystem::exists(path)); +} + 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 df10b2985b..23c42a944e 100644 --- a/tests/libslic3r/test_utils.cpp +++ b/tests/libslic3r/test_utils.cpp @@ -93,6 +93,10 @@ TEST_CASE("write_file_atomically reports a missing directory and writes nothing" } 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")); @@ -164,33 +168,33 @@ TEST_CASE("write_file_atomically survives two threads writing one target", "[Uti REQUIRE(temporaries == 0); } -TEST_CASE("remove_stale_temp_files removes only old ..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", "a.json.99", "a.json.12.3.tmp" }) { + for (const char *name : { "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" }) { 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")); + REQUIRE_FALSE(write_file_atomically((dir.path() / "g.json.7.2.tmp").string(), "x")); - SECTION("with a name prefix only matching names go, and a numbered backup is not a temporary") { - REQUIRE(remove_stale_temp_files(dir.path(), "a.json") == 2); - REQUIRE_FALSE(boost::filesystem::exists(dir.path() / "a.json.123.tmp")); - REQUIRE_FALSE(boost::filesystem::exists(dir.path() / "a.json.12.3.tmp")); + SECTION("with a name prefix only matching names go; a numbered backup or a one-segment name is not a temporary") { + 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.99")); - REQUIRE(boost::filesystem::exists(dir.path() / "b.info.4.tmp")); + 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()) == 3); + 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 == 6); + REQUIRE(entries == 7); REQUIRE(boost::filesystem::exists(dir.path() / "a.json.99")); - REQUIRE(boost::filesystem::exists(dir.path() / "g.json.7.tmp")); + REQUIRE(boost::filesystem::exists(dir.path() / "g.json.7.2.tmp")); } } diff --git a/tests/libslic3r/test_vendor_cache.cpp b/tests/libslic3r/test_vendor_cache.cpp index c290a38285..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,6 +1362,10 @@ 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"));