diff --git a/src/libslic3r/AppConfig.cpp b/src/libslic3r/AppConfig.cpp index 2012a72aee..f1a711f40c 100644 --- a/src/libslic3r/AppConfig.cpp +++ b/src/libslic3r/AppConfig.cpp @@ -1121,8 +1121,10 @@ void AppConfig::save() j["local_machines"][local_machine.first] = m_json; } const std::string config_str = j.dump(1, '\t'); - if (write_config_file(path, config_str + "\n", config_str)) - m_dirty = false; + write_config_file(path, config_str + "\n", config_str); + // Cleared even after a failed write, as before: the failure is logged, and + // the next change tries again rather than every idle tick. + m_dirty = false; } #else @@ -1309,12 +1311,12 @@ void AppConfig::save() config_ss << std::endl; const std::string config_str = config_ss.str(); - if (write_config_file(path, config_str, config_str)) - m_dirty = false; + 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) +void 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()); @@ -1328,11 +1330,8 @@ bool AppConfig::write_config_file(const std::string &path, std::string body, con BOOST_LOG_TRIVIAL(error) << "Failed to write the backup configuration " << backup_path << ": " << ec.message(); #endif // The config was always replaced, never written in place, so a read-only one is replaced still. - if (const std::error_code ec = write_file_atomically(path, body, false, /*replace_read_only=*/true)) { + 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(); - return false; - } - return true; } bool AppConfig::get_variant(const std::string &vendor, const std::string &model, const std::string &variant) const diff --git a/src/libslic3r/AppConfig.hpp b/src/libslic3r/AppConfig.hpp index 29e8353b4c..abfad5fc7a 100644 --- a/src/libslic3r/AppConfig.hpp +++ b/src/libslic3r/AppConfig.hpp @@ -453,7 +453,7 @@ private: MachineSettingMap m_printer_settings; // Writes the assembled config text, and on Windows its checksum and a backup copy. `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 write_config_file(const std::string &path, std::string body, const std::string &checksum_source); bool m_dirty; // Original version found in the ini file before it was overwritten diff --git a/src/libslic3r/InstanceLock.cpp b/src/libslic3r/InstanceLock.cpp index 2cfe7ac8f9..da787f8caf 100644 --- a/src/libslic3r/InstanceLock.cpp +++ b/src/libslic3r/InstanceLock.cpp @@ -6,6 +6,7 @@ #include #include +#include #include #ifdef _WIN32 #include @@ -26,7 +27,10 @@ struct InstanceLock::Slot 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. + // Checked at most every few seconds, since a preset scan takes one guard + // per file. std::string identity; + std::chrono::steady_clock::time_point identity_checked_at{}; int depth{0}; bool file_locked{false}; // After a timed-out wait, guards skip waiting until this point. @@ -55,14 +59,20 @@ InstanceLock::Slot &InstanceLock::slot_for(const std::string &lock_file_path) bool InstanceLock::open_lock_file(Slot &slot, const std::string &lock_file_path) { try { - // file_lock only opens existing files. + // file_lock only opens existing files, read-write, and every user + // sharing the data dir has to be able to open it. boost::nowide::ofstream(lock_file_path, std::ios::app).close(); + boost::system::error_code ec; + boost::filesystem::permissions(lock_file_path, boost::filesystem::owner_read | boost::filesystem::owner_write | + boost::filesystem::group_read | boost::filesystem::group_write | + boost::filesystem::others_read | boost::filesystem::others_write, ec); #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 - slot.identity = file_identity(lock_file_path); + slot.identity = file_identity(lock_file_path); + slot.identity_checked_at = std::chrono::steady_clock::now(); return true; } catch (const std::exception &e) { slot.retry_at = std::chrono::steady_clock::now() + cooldown; @@ -81,9 +91,12 @@ InstanceLock::InstanceLock(const std::string &lock_file_path, std::chrono::milli 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 && m_slot->file_lock && now - m_slot->identity_checked_at >= identity_check_interval) { + m_slot->identity_checked_at = now; + if (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))) { diff --git a/src/libslic3r/InstanceLock.hpp b/src/libslic3r/InstanceLock.hpp index 94926d2292..fa20153e8b 100644 --- a/src/libslic3r/InstanceLock.hpp +++ b/src/libslic3r/InstanceLock.hpp @@ -36,6 +36,9 @@ public: // 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}; + // How often a guard re-checks that the lock file behind the path is still + // the one it opened. Mutable so tests can shorten it. + static inline std::chrono::milliseconds identity_check_interval{5000}; // 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 d96aa4a766..7042db1cdb 100644 --- a/src/libslic3r/Preset.cpp +++ b/src/libslic3r/Preset.cpp @@ -4375,6 +4375,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. diff --git a/src/libslic3r/utils.cpp b/src/libslic3r/utils.cpp index eb55ea46fa..2b1135dee0 100644 --- a/src/libslic3r/utils.cpp +++ b/src/libslic3r/utils.cpp @@ -14,12 +14,6 @@ #include #include #include -#ifdef _WIN32 -#include -#else -#include -#include -#endif #include #include @@ -44,6 +38,7 @@ #else #include #include + #include #include #include #ifdef BSD @@ -54,7 +49,6 @@ #include #endif #ifdef __linux__ - #include #include #include #include @@ -712,7 +706,16 @@ namespace WindowsSupport std::error_code rename_file(const std::string &from, const std::string &to) { #ifdef _WIN32 - return WindowsSupport::rename(from, to); + std::error_code ec = WindowsSupport::rename(from, to); + // 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. A target that is not + // writable at all is refused for good and not worth waiting on. + for (int attempt = 0; attempt < 20 && ec == std::errc::permission_denied && ::_waccess(boost::nowide::widen(to).c_str(), 2) == 0; ++ attempt) { + std::this_thread::sleep_for(std::chrono::milliseconds(25)); + ec = WindowsSupport::rename(from, to); + } + return ec; #else // rename(2) replaces an existing target atomically; removing it first would // leave a window in which the file does not exist at all. @@ -720,14 +723,19 @@ std::error_code rename_file(const std::string &from, const std::string &to) return {}; const int err = errno; // Some mounts (sshfs, gvfs, MTP and a few SMB setups) refuse to replace an - // existing target in one step and report it as one of these; take the old - // two-step route there, but not for an I/O error that would only lose the - // target for nothing. + // existing target in one step and report it as one of these; move the + // target aside and try again there, and put it back if that fails too, so + // a refusal that was really about the source never costs the target. const bool replace_refused = err == EPERM || err == EACCES || err == EEXIST || err == ENOTEMPTY || err == EBUSY || err == ENOTSUP || err == EOPNOTSUPP; - if (replace_refused && boost::nowide::remove(to.c_str()) == 0) { - if (boost::nowide::rename(from.c_str(), to.c_str()) == 0) - return {}; - BOOST_LOG_TRIVIAL(error) << "Replacing " << to << " failed after the old file was removed: " << std::strerror(errno); + if (replace_refused) { + const std::string aside = to + "." + std::to_string(get_current_pid()) + ".old"; + if (boost::nowide::rename(to.c_str(), aside.c_str()) == 0) { + if (boost::nowide::rename(from.c_str(), to.c_str()) == 0) { + boost::nowide::remove(aside.c_str()); + return {}; + } + boost::nowide::rename(aside.c_str(), to.c_str()); + } } return std::make_error_code(static_cast(err)); #endif @@ -789,22 +797,12 @@ std::error_code write_file_atomically(const std::string &path, std::initializer_ if (target_exists) boost::filesystem::permissions(tmp_path, target.permissions(), bec); #endif - std::error_code ec = rename_file(tmp_path, path); -#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; a - // reader is done in milliseconds, so wait it out before giving up on the - // atomic path. Elsewhere a refused replace does not clear by waiting. - for (int attempt = 0; attempt < 20 && ec == std::errc::permission_denied; ++ attempt) { - std::this_thread::sleep_for(std::chrono::milliseconds(25)); - ec = rename_file(tmp_path, path); - } -#endif - if (ec) { + if (const std::error_code ec = rename_file(tmp_path, path)) { boost::nowide::remove(tmp_path.c_str()); - // 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. + // A reader on Windows that outlasted rename_file()'s wait, 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_whole_file(path, chunks, binary); } diff --git a/tests/libslic3r/test_instance_lock.cpp b/tests/libslic3r/test_instance_lock.cpp index 6ff9cca250..b5dd4818a1 100644 --- a/tests/libslic3r/test_instance_lock.cpp +++ b/tests/libslic3r/test_instance_lock.cpp @@ -100,6 +100,8 @@ TEST_CASE("InstanceLock reopens a lock file that was replaced on disk", "[Instan { ScopedTemporaryFile lock_file(".lock"); const std::string path = lock_file.string(); + const auto saved_interval = InstanceLock::identity_check_interval; + InstanceLock::identity_check_interval = 0ms; { InstanceLock lock(path); REQUIRE(lock.locked()); @@ -107,6 +109,7 @@ TEST_CASE("InstanceLock reopens a lock file that was replaced on disk", "[Instan boost::filesystem::remove(path); InstanceLock lock(path); + InstanceLock::identity_check_interval = saved_interval; 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 diff --git a/tests/libslic3r/test_utils.cpp b/tests/libslic3r/test_utils.cpp index bd9d5afc49..e75c34d28b 100644 --- a/tests/libslic3r/test_utils.cpp +++ b/tests/libslic3r/test_utils.cpp @@ -20,7 +20,7 @@ using namespace Slic3r; -TEST_CASE("per_user_temp_dir composes a per-user temp root", "[Utils]") { +TEST_CASE("per_user_temp_dir composes a per-user temp root", "[utils]") { const std::string base = "/tmp"; SECTION("an empty id returns base unchanged") { @@ -34,7 +34,7 @@ TEST_CASE("per_user_temp_dir composes a per-user temp root", "[Utils]") { } } -TEST_CASE("per_user_temp_id follows the platform contract", "[Utils]") { +TEST_CASE("per_user_temp_id follows the platform contract", "[utils]") { const std::string id = per_user_temp_id(); SECTION("stable across calls") { @@ -54,7 +54,7 @@ TEST_CASE("per_user_temp_id follows the platform contract", "[Utils]") { // The end-to-end contract callers depend on: the temp root is left alone on // Windows and isolated per user on Linux/macOS. -TEST_CASE("per-user temp root is unchanged on Windows, isolated elsewhere", "[Utils]") { +TEST_CASE("per-user temp root is unchanged on Windows, isolated elsewhere", "[utils]") { const std::string base = "/tmp"; const std::string root = per_user_temp_dir(base, per_user_temp_id()); #ifdef _WIN32 @@ -65,7 +65,7 @@ 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]") { +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"; @@ -83,7 +83,7 @@ TEST_CASE("write_file_atomically replaces the target and leaves no temporary fil REQUIRE(entries == 1); } -TEST_CASE("write_file_atomically reports a missing directory and writes nothing", "[Utils]") { +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"; @@ -92,7 +92,7 @@ TEST_CASE("write_file_atomically reports a missing directory and writes nothing" REQUIRE_FALSE(boost::filesystem::exists(target)); } -TEST_CASE("write_file_atomically refuses a read-only target and leaves it untouched", "[Utils]") { +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"); @@ -111,7 +111,7 @@ TEST_CASE("write_file_atomically refuses a read-only target and leaves it untouc REQUIRE(content == "pinned"); } -TEST_CASE("write_file_atomically replaces a read-only target when asked to", "[Utils]") { +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"); @@ -130,7 +130,7 @@ TEST_CASE("write_file_atomically replaces a read-only target when asked to", "[U REQUIRE(content == "replaced"); } -TEST_CASE("write_file_atomically keeps bytes intact in binary mode", "[Utils]") { +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); @@ -140,7 +140,7 @@ TEST_CASE("write_file_atomically keeps bytes intact in binary mode", "[Utils]") } #ifndef _WIN32 -TEST_CASE("write_file_atomically writes through a symlink and keeps the target's permissions", "[Utils]") { +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"; @@ -161,7 +161,7 @@ TEST_CASE("write_file_atomically writes through a symlink and keeps the target's } #endif -TEST_CASE("write_file_atomically survives two threads writing one target", "[Utils]") { +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'); @@ -187,7 +187,7 @@ 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.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")); @@ -217,7 +217,7 @@ TEST_CASE("remove_stale_temp_files removes only old ...tmp files", } } -TEST_CASE("copy_file reports the OS error when the destination cannot be written", "[Utils]") { +TEST_CASE("copy_file reports the OS error when the destination cannot be written", "[utils]") { ScopedTemporaryFile source(".txt"); { std::ofstream ofs(source.string(), std::ios::binary); @@ -246,7 +246,7 @@ TEST_CASE("copy_file reports the OS error when the destination cannot be written #endif // _WIN32 } -TEST_CASE("A resolved input path still names the same file after the working directory changes", "[Utils]") { +TEST_CASE("A resolved input path still names the same file after the working directory changes", "[utils]") { ScopedTemporaryFile model(".3mf"); { std::ofstream out(model.string()); out << "3mf"; } const std::string name = model.path().filename().string(); @@ -263,7 +263,7 @@ TEST_CASE("A resolved input path still names the same file after the working dir REQUIRE_FALSE(boost::filesystem::exists(name)); } -TEST_CASE("resolve_cli_input_path completes a relative path against the working directory", "[Utils]") { +TEST_CASE("resolve_cli_input_path completes a relative path against the working directory", "[utils]") { ScopedWorkingDirectory cwd(boost::filesystem::temp_directory_path()); // Read back rather than reusing temp_directory_path(): changing to it resolves any symlink. const boost::filesystem::path here = boost::filesystem::current_path(); @@ -279,7 +279,7 @@ TEST_CASE("resolve_cli_input_path completes a relative path against the working } } -TEST_CASE("resolve_cli_input_path leaves inputs that must not be completed unchanged", "[Utils]") { +TEST_CASE("resolve_cli_input_path leaves inputs that must not be completed unchanged", "[utils]") { SECTION("an absolute path") { const boost::filesystem::path absolute = (boost::filesystem::temp_directory_path() / "model.3mf").make_preferred(); REQUIRE(resolve_cli_input_path(absolute.string()) == absolute.string());