diff --git a/src/libslic3r/AppConfig.cpp b/src/libslic3r/AppConfig.cpp index 680583f438..5dd1bd045f 100644 --- a/src/libslic3r/AppConfig.cpp +++ b/src/libslic3r/AppConfig.cpp @@ -1323,17 +1323,23 @@ bool AppConfig::write_config_file(const std::string &path, std::string body, con // 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. + 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 " << m_retry_save_after.count() << " s"; + m_retry_save_at = std::chrono::steady_clock::now() + m_retry_save_after; + m_retry_save_after = std::min(m_retry_save_after * 2, std::chrono::seconds(300)); + return false; + } + m_retry_save_at = {}; + m_retry_save_after = std::chrono::seconds(10); +#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 - // 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)) { - 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 = {}; return true; } diff --git a/src/libslic3r/AppConfig.hpp b/src/libslic3r/AppConfig.hpp index 5d2ecd9d16..ad92ac67ce 100644 --- a/src/libslic3r/AppConfig.hpp +++ b/src/libslic3r/AppConfig.hpp @@ -456,13 +456,16 @@ 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` + // 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); + // 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 until this point. + // After a failed write, save_due() is false until this point, which moves out + // ten seconds, then twenty, up to five minutes, for every failure in a row. std::chrono::steady_clock::time_point m_retry_save_at{}; + std::chrono::seconds m_retry_save_after{10}; // 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/InstanceLock.cpp b/src/libslic3r/InstanceLock.cpp index f030caef19..d5aede0c38 100644 --- a/src/libslic3r/InstanceLock.cpp +++ b/src/libslic3r/InstanceLock.cpp @@ -5,27 +5,63 @@ #include #include -#include -#include #include +#include #include #ifdef _WIN32 +#include #include +#else +#include +#include +#include +#include #endif +#include #include "Utils.hpp" namespace Slic3r { -// One slot per lock file, shared by every guard in the process. A POSIX fcntl -// lock is owned by the process, not the thread or descriptor, so a single -// file_lock 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. +#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_CLOEXEC)) + { + 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. struct InstanceLock::Slot { std::recursive_mutex mutex; // Absent until the lock file has been created and opened. - std::unique_ptr file_lock; + 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; @@ -60,17 +96,17 @@ 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, read-write, and every user - // sharing the data dir has to be able to open it. + // The lock opens an existing file 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()); + 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()); + slot.file_lock = std::make_unique(lock_file_path.c_str()); #endif slot.identity = file_identity(lock_file_path); slot.identity_checked_at = std::chrono::steady_clock::now(); diff --git a/src/libslic3r/InstanceLock.hpp b/src/libslic3r/InstanceLock.hpp index 9d8d3b795a..c998abca29 100644 --- a/src/libslic3r/InstanceLock.hpp +++ b/src/libslic3r/InstanceLock.hpp @@ -9,7 +9,9 @@ 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`, created on first use +// through an advisory OS file lock on `lock_file_path` (flock on POSIX, tied to +// the guard's own open file so nothing else in the process can drop it by +// opening and closing the file; LockFileEx on Windows), created on first use // and kept: the lock state lives in the kernel on the open file, so deleting // the file on release would let a third instance lock a fresh file while the // second still holds the old one. The OS releases the lock when its holder @@ -41,10 +43,10 @@ public: // 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: on every acquisition by default, one stat, since a - // handle to a replaced file would lock nothing anyone else can see. - // Mutable so tests can change it. - static inline std::chrono::milliseconds identity_check_interval{0}; + // the one it opened, since a handle to a replaced file would lock nothing + // anyone else can see: a stat once a second bounds that window while a scan + // of hundreds of presets pays for it once. Mutable so tests can change it. + static inline std::chrono::milliseconds identity_check_interval{1000}; // 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/Utils.hpp b/src/libslic3r/Utils.hpp index 906ab39031..69551b19af 100644 --- a/src/libslic3r/Utils.hpp +++ b/src/libslic3r/Utils.hpp @@ -235,16 +235,16 @@ extern std::error_code rename_file(const std::string &from, const std::string &t // 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 successful write into a directory under data_dir() also removes stale -// leftovers there, at most once an hour per directory. +// A successful write into a directory under data_dir() also sweeps stale +// leftovers there. extern std::error_code write_file_atomically(const std::string &path, std::initializer_list chunks, bool binary = false, bool replace_read_only = false); inline std::error_code write_file_atomically(const std::string &path, const std::string &content, bool binary = false, bool replace_read_only = false) { return write_file_atomically(path, { std::string_view(content) }, binary, replace_read_only); } -// Remove the `...tmp` files a crashed write_file_atomically() and -// the `..old` files a crashed rename_file() left in `dir` at least an -// hour ago, only names starting with `name_prefix` when it is given; an `.old` -// whose `` is missing is the last copy and is put back instead. Returns how -// many were removed. +// Remove the `...tmp` files a crashed write_file_atomically() left +// in `dir` at least an hour ago, only names starting with `name_prefix` when it is +// given, and log the `..old` files a crashed rename_file() left, which +// may be a last copy or a since-deleted file and so stay for the user. Runs at +// most once an hour per directory and prefix. 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. diff --git a/src/libslic3r/utils.cpp b/src/libslic3r/utils.cpp index bd04799854..74909d0a98 100644 --- a/src/libslic3r/utils.cpp +++ b/src/libslic3r/utils.cpp @@ -458,6 +458,12 @@ boost::filesystem::path get_log_file_name() #ifdef _WIN32 // The following helpers are borrowed from the LLVM project https://github.com/llvm +// Names a file object by volume and file index, the same identity file_identity() reports. +static std::string file_identity_of(const BY_HANDLE_FILE_INFORMATION &info) +{ + return std::to_string(info.dwVolumeSerialNumber) + ":" + std::to_string((static_cast(info.nFileIndexHigh) << 32) | info.nFileIndexLow); +} + namespace WindowsSupport { template @@ -679,7 +685,7 @@ namespace WindowsSupport BY_HANDLE_FILE_INFORMATION FI2; if (! ::GetFileInformationByHandle(to_handle2, &FI2)) return map_windows_error(GetLastError()); - if (FI.nFileIndexHigh != FI2.nFileIndexHigh || FI.nFileIndexLow != FI2.nFileIndexLow || FI.dwVolumeSerialNumber != FI2.dwVolumeSerialNumber) + if (file_identity_of(FI) != file_identity_of(FI2)) break; continue; } @@ -740,10 +746,14 @@ std::error_code rename_file(const std::string &from, const std::string &to) static bool is_writable(const std::string &path) { #ifdef _WIN32 - // _waccess() sees only the read-only attribute; an open for writing sees ACLs too. + // _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) - return false; + if (handle == INVALID_HANDLE_VALUE) { + const DWORD err = ::GetLastError(); + return err == ERROR_SHARING_VIOLATION || err == ERROR_LOCK_VIOLATION; + } ::CloseHandle(handle); return true; #else @@ -813,25 +823,10 @@ std::error_code write_file_atomically(const std::string &path, std::initializer_ return write_whole_file(path, chunks, binary); } // Crash leftovers of earlier writes into this directory, for the files the - // application owns; once an hour per directory, so a batch of saves does - // not read the directory once per file. + // application owns; the sweep throttles itself per directory. const boost::filesystem::path dir = boost::filesystem::path(path).parent_path(); - if (! data_dir().empty() && boost::algorithm::starts_with(dir.generic_string(), boost::filesystem::path(data_dir()).generic_string())) { - static std::mutex swept_mutex; - static std::map swept_at; - const auto now = std::chrono::steady_clock::now(); - bool sweep = false; - { - std::lock_guard guard(swept_mutex); - auto &last = swept_at[dir.string()]; - if (last == std::chrono::steady_clock::time_point{} || now - last >= std::chrono::hours(1)) { - last = now; - sweep = true; - } - } - if (sweep) - remove_stale_temp_files(dir); - } + if (! data_dir().empty() && boost::algorithm::starts_with(dir.generic_string(), boost::filesystem::path(data_dir()).generic_string())) + remove_stale_temp_files(dir); return {}; } @@ -847,7 +842,7 @@ std::string file_identity(const std::string &path) ::CloseHandle(handle); if (! ok) return {}; - return std::to_string(info.dwVolumeSerialNumber) + ":" + std::to_string((static_cast(info.nFileIndexHigh) << 32) | info.nFileIndexLow); + return file_identity_of(info); #else struct stat st; if (::stat(path.c_str(), &st) != 0) @@ -858,6 +853,18 @@ std::string file_identity(const std::string &path) size_t remove_stale_temp_files(const boost::filesystem::path &dir, const std::string &name_prefix) { + // Once an hour per directory: a batch of saves or a load followed by a + // save must not read the same directory over and over. + { + static std::mutex swept_mutex; + static std::map swept_at; + const auto now = std::chrono::steady_clock::now(); + std::lock_guard guard(swept_mutex); + auto &last = swept_at[dir.string() + "|" + name_prefix]; + if (last != std::chrono::steady_clock::time_point{} && now - last < std::chrono::hours(1)) + return 0; + last = now; + } // .....tmp, exactly the shape write_file_atomically() // makes, or ....old, the shape rename_file() moves a // target aside under. @@ -900,14 +907,11 @@ size_t remove_stale_temp_files(const boost::filesystem::path &dir, const std::st continue; const std::string name = it->path().filename().string(); if (name.size() > 4 && name.compare(name.size() - 4, 4, ".old") == 0) { - // A target rename_file() moved aside and never put back or removed: - // the last copy of the file if the target is missing, so it goes back. - const boost::filesystem::path original = it->path().parent_path() / name.substr(0, name.rfind('.', name.size() - 5)); - if (! boost::filesystem::exists(original, entry_ec)) { - boost::filesystem::rename(it->path(), original, entry_ec); - BOOST_LOG_TRIVIAL(warning) << "Restored " << original << " from " << it->path() << (entry_ec ? ": failed" : ""); - continue; - } + // A target rename_file() moved aside and never put back or removed. + // It may be the last copy of a file, or a file since deleted on + // purpose; neither removing nor restoring it is safe unasked. + BOOST_LOG_TRIVIAL(warning) << it->path() << " was set aside by an interrupted save; restore or delete it by hand"; + continue; } if (boost::filesystem::remove(it->path(), entry_ec)) { BOOST_LOG_TRIVIAL(info) << "Removed stale temporary file " << it->path(); diff --git a/tests/libslic3r/test_instance_lock.cpp b/tests/libslic3r/test_instance_lock.cpp index 3cd3a0a7d9..ad24acd0fa 100644 --- a/tests/libslic3r/test_instance_lock.cpp +++ b/tests/libslic3r/test_instance_lock.cpp @@ -11,6 +11,7 @@ #ifndef _WIN32 #include +#include #include #include #endif @@ -149,9 +150,9 @@ TEST_CASE("InstanceLock serialises the threads of one process", "[InstanceLock]" } #ifndef _WIN32 -// The cross-process side of the lock is a POSIX fcntl write lock, which a -// child process takes here directly; the same primitive backs the guard on -// Windows through LockFileEx, but spawning a child there is not worth a test. +// 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"); @@ -164,11 +165,8 @@ TEST_CASE("InstanceLock yields to another process and reports it", "[InstanceLoc const pid_t child = ::fork(); REQUIRE(child >= 0); if (child == 0) { - int fd = ::open(path.c_str(), O_RDWR | O_CREAT, 0644); - struct flock lock{}; - lock.l_type = F_WRLCK; - lock.l_whence = SEEK_SET; - char byte = ::fcntl(fd, F_SETLK, &lock) == 0 ? '1' : '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); diff --git a/tests/libslic3r/test_utils.cpp b/tests/libslic3r/test_utils.cpp index 7c1fddf823..f32c43b612 100644 --- a/tests/libslic3r/test_utils.cpp +++ b/tests/libslic3r/test_utils.cpp @@ -187,17 +187,14 @@ TEST_CASE("write_file_atomically survives two threads writing one target", "[uti REQUIRE(temporaries == 0); } -TEST_CASE("remove_stale_temp_files puts back a moved-aside file whose original is missing", "[utils]") { +TEST_CASE("remove_stale_temp_files leaves a moved-aside file to the user", "[utils]") { ScopedTemporaryDir dir; - REQUIRE_FALSE(write_file_atomically((dir.path() / "lost.json.4242.old").string(), "last copy")); + REQUIRE_FALSE(write_file_atomically((dir.path() / "lost.json.4242.old").string(), "maybe the last copy")); boost::filesystem::last_write_time(dir.path() / "lost.json.4242.old", std::time(nullptr) - 7200); REQUIRE(remove_stale_temp_files(dir.path()) == 0); - REQUIRE(boost::filesystem::exists(dir.path() / "lost.json")); - REQUIRE_FALSE(boost::filesystem::exists(dir.path() / "lost.json.4242.old")); - std::string content; - load_string_file(dir.path() / "lost.json", content); - REQUIRE(content == "last copy"); + REQUIRE(boost::filesystem::exists(dir.path() / "lost.json.4242.old")); + REQUIRE_FALSE(boost::filesystem::exists(dir.path() / "lost.json")); } TEST_CASE("remove_stale_temp_files removes only old ...tmp files", "[utils]") { @@ -211,22 +208,23 @@ TEST_CASE("remove_stale_temp_files removes only old ...tmp files", // 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 or a one-segment name is not a temporary") { - REQUIRE(remove_stale_temp_files(dir.path(), "a.json") == 2); + SECTION("with a name prefix only matching names go; a numbered backup, a one-segment name or an .old 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_FALSE(boost::filesystem::exists(dir.path() / "a.json.123.old")); + REQUIRE(boost::filesystem::exists(dir.path() / "a.json.123.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()) == 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 == 9); + REQUIRE(entries == 10); + REQUIRE(boost::filesystem::exists(dir.path() / "a.json.123.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"));