diff --git a/src/libslic3r/InstanceLock.cpp b/src/libslic3r/InstanceLock.cpp index d5aede0c38..e25db448eb 100644 --- a/src/libslic3r/InstanceLock.cpp +++ b/src/libslic3r/InstanceLock.cpp @@ -3,6 +3,7 @@ #include #include #include +#include #include #include @@ -17,9 +18,6 @@ #include #include #endif -#include - -#include "Utils.hpp" namespace Slic3r { @@ -56,16 +54,14 @@ private: // 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. +// to use from the preset sync thread and the GUI thread at once. The lock +// file is opened by the outermost guard and closed when it goes, so the file +// is never held open between guards: whatever is at the path is what gets +// locked, and a data dir can be removed once nothing is saving into it. 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; - 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; each wait @@ -73,14 +69,14 @@ struct InstanceLock::Slot std::chrono::steady_clock::time_point skip_waiting_until{}; int consecutive_timeouts{0}; // After a failed open or a failing lock call, guards skip the file lock - // entirely until this point. + // entirely until this point, again doubling for failures in a row. std::chrono::steady_clock::time_point retry_at{}; + int consecutive_failures{0}; }; InstanceLock::Slot &InstanceLock::slot_for(const std::string &lock_file_path) { - // Never freed: the slots keep the lock files open for as long as anything - // in the process may still save, including during static destruction. + // Never freed: a save during static destruction still needs its slot. static auto *registry_mutex = new std::mutex(); static auto *registry = new std::map>(); @@ -91,6 +87,11 @@ InstanceLock::Slot &InstanceLock::slot_for(const std::string &lock_file_path) return *slot; } +static std::chrono::milliseconds backoff_for(int failures_in_a_row, std::chrono::milliseconds base) +{ + return std::min(base * (1 << std::min(failures_in_a_row, 5)), std::chrono::milliseconds(std::chrono::minutes(5))); +} + // Creates the lock file if needed and opens it, or starts the cool-down. // Called with the slot mutex held. bool InstanceLock::open_lock_file(Slot &slot, const std::string &lock_file_path) @@ -108,14 +109,13 @@ 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); - 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; + const auto backoff = backoff_for(slot.consecutive_failures ++, cooldown); + slot.retry_at = std::chrono::steady_clock::now() + backoff; BOOST_LOG_TRIVIAL(warning) << "Cannot open lock file " << lock_file_path << ": " << e.what() << " (check its owner and permissions); other instances are not excluded from writing for the next " - << cooldown.count() << " ms"; + << backoff.count() << " ms"; return false; } } @@ -127,17 +127,7 @@ 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 && 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))) { + if (m_slot->depth == 0 && now >= m_slot->retry_at && open_lock_file(*m_slot, lock_file_path)) { const bool wait = now >= m_slot->skip_waiting_until; const auto deadline = now + timeout; for (;;) { @@ -145,20 +135,21 @@ InstanceLock::InstanceLock(const std::string &lock_file_path, std::chrono::milli if (m_slot->file_lock->try_lock()) { m_slot->file_locked = true; m_slot->consecutive_timeouts = 0; + m_slot->consecutive_failures = 0; m_slot->skip_waiting_until = {}; break; } } catch (const std::exception &e) { - m_slot->retry_at = std::chrono::steady_clock::now() + cooldown; + const auto backoff = backoff_for(m_slot->consecutive_failures ++, cooldown); + m_slot->retry_at = std::chrono::steady_clock::now() + backoff; BOOST_LOG_TRIVIAL(warning) << "Cannot lock " << lock_file_path << ": " << e.what() - << "; proceeding without the lock for the next " << cooldown.count() << " ms"; + << "; proceeding without the lock for the next " << backoff.count() << " ms"; break; } if (! wait) break; if (std::chrono::steady_clock::now() >= deadline) { - const auto backoff = std::min(cooldown * (1 << std::min(m_slot->consecutive_timeouts, 5)), std::chrono::milliseconds(std::chrono::minutes(5))); - ++ m_slot->consecutive_timeouts; + const auto backoff = backoff_for(m_slot->consecutive_timeouts ++, cooldown); m_slot->skip_waiting_until = deadline + backoff; BOOST_LOG_TRIVIAL(warning) << "Another instance has held " << lock_file_path << " for over " << timeout.count() << " ms; proceeding without the lock for the next " @@ -167,6 +158,8 @@ InstanceLock::InstanceLock(const std::string &lock_file_path, std::chrono::milli } std::this_thread::sleep_for(std::chrono::milliseconds(5)); } + if (! m_slot->file_locked) + m_slot->file_lock.reset(); } // Counted last, so a throw above leaves the slot exactly as it was found. ++ m_slot->depth; @@ -177,13 +170,16 @@ InstanceLock::~InstanceLock() { if (m_slot == nullptr) return; - if (-- m_slot->depth == 0 && m_slot->file_locked) { - try { - m_slot->file_lock->unlock(); - } catch (const std::exception &e) { - BOOST_LOG_TRIVIAL(warning) << "Cannot unlock instance lock: " << e.what(); + if (-- m_slot->depth == 0 && m_slot->file_lock) { + if (m_slot->file_locked) { + try { + m_slot->file_lock->unlock(); + } catch (const std::exception &e) { + BOOST_LOG_TRIVIAL(warning) << "Cannot unlock instance lock: " << e.what(); + } + m_slot->file_locked = false; } - m_slot->file_locked = false; + m_slot->file_lock.reset(); } } diff --git a/src/libslic3r/InstanceLock.hpp b/src/libslic3r/InstanceLock.hpp index c998abca29..36ad089f31 100644 --- a/src/libslic3r/InstanceLock.hpp +++ b/src/libslic3r/InstanceLock.hpp @@ -11,11 +11,14 @@ namespace Slic3r { // of this process are serialised through a recursive mutex, other processes // 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 -// exits, so a crashed instance never leaves a stale lock behind. +// opening and closing the file; LockFileEx on Windows). The file is created on +// first use and kept, since the lock state lives in the kernel on the open +// file and deleting the file on release would let a third instance lock a +// fresh file while the second still holds the old one; it is opened by the +// outermost guard and closed when that guard goes, so whatever is at the path +// is what gets locked and nothing stays open between saves. The OS releases +// the lock when its holder exits, so a crashed instance never leaves a stale +// lock behind. // // The lock is best effort: when the lock file cannot be created, or another // instance still holds it after `timeout` (a second by default), the guard keeps only the in-process @@ -26,7 +29,10 @@ namespace Slic3r { // pays the wait once rather than once per file and a holder that never lets go // does not cost a stall every cool-down for good; 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`. +// service), is likewise retried only after `cooldown`, doubling the same way. +// A wait for the in-process mutex is bounded only by the longest critical +// section, which is why every guard covers a few file operations and nothing +// slower, such as removing a directory tree. // // 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. @@ -42,11 +48,6 @@ 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, 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/Preset.cpp b/src/libslic3r/Preset.cpp index a1773cd805..676d5c37f8 100644 --- a/src/libslic3r/Preset.cpp +++ b/src/libslic3r/Preset.cpp @@ -112,6 +112,38 @@ std::string user_presets_lock_path(bool read_only) return read_only || data_dir().empty() ? std::string() : (fs::path(data_dir()) / (PRESET_USER_DIR ".lock")).string(); } +// Removes a preset file the scan could not load, and its .info, under the lock. +static void remove_preset_files(const std::string &preset_file, bool read_only) +{ + if (read_only) + return; + InstanceLock instance_lock(user_presets_lock_path()); + boost::system::error_code ec; + fs::path file_path(preset_file); + fs::remove(file_path, ec); + file_path.replace_extension(".info"); + fs::remove(file_path, ec); +} + +void remove_directory_tree_locked(const boost::filesystem::path &dir) +{ + boost::system::error_code ec; + fs::path doomed = dir; + doomed += "." + std::to_string(get_current_pid()) + ".removing"; + { + InstanceLock instance_lock(user_presets_lock_path()); + if (! fs::exists(dir, ec)) + return; + fs::rename(dir, doomed, ec); + if (ec) { + // Cannot be set aside: the slow way, still under the lock. + fs::remove_all(dir, ec); + return; + } + } + fs::remove_all(doomed, ec); +} + std::string get_preset_bare_name(const std::string &canonical_name) { const auto pos = canonical_name.find_last_of('/'); @@ -1770,12 +1802,7 @@ void PresetCollection::load_presets( //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); + remove_preset_files(preset.file, read_only); BOOST_LOG_TRIVIAL(error) << boost::format("parse config %1% failed")%preset.file; ++m_errors; continue; @@ -1866,25 +1893,13 @@ void PresetCollection::load_presets( } catch (const std::ifstream::failure &err) { ++m_errors; BOOST_LOG_TRIVIAL(error) << boost::format("The user-config cannot be loaded: %1%. Reason: %2%")%preset.file %err.what(); - InstanceLock instance_lock(user_presets_lock_path(read_only)); - 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); + remove_preset_files(preset.file, read_only); //throw Slic3r::RuntimeError(std::string("The selected preset cannot be loaded: ") + preset.file + "\n\tReason: " + err.what()); } catch (const std::runtime_error &err) { ++m_errors; BOOST_LOG_TRIVIAL(error) << boost::format("Failed loading the user-config file: %1%. Reason: %2%")%preset.file %err.what(); //throw Slic3r::RuntimeError(std::string("Failed loading the preset file: ") + preset.file + "\n\tReason: " + err.what()); - InstanceLock instance_lock(user_presets_lock_path(read_only)); - 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); + remove_preset_files(preset.file, read_only); } if (preset_loaded_fn != nullptr) diff --git a/src/libslic3r/Preset.hpp b/src/libslic3r/Preset.hpp index 7fbc3b77dc..8ff92859d9 100644 --- a/src/libslic3r/Preset.hpp +++ b/src/libslic3r/Preset.hpp @@ -490,6 +490,12 @@ std::string get_preset_bare_name(const std::string &canonical_name); // many jobs on one data dir. std::string user_presets_lock_path(bool read_only = false); +// Removes a directory tree under the user preset lock without holding the lock +// for the removal itself: the tree is renamed aside under the lock, in one +// step, and deleted afterwards, so a save waiting on the lock waits +// milliseconds rather than for a tree of files to go. +void remove_directory_tree_locked(const boost::filesystem::path &dir); + // Resolve an origin from a directory path when the caller passes Kind::Auto. PresetOrigin detect_origin_from_path(const boost::filesystem::path &path, const PresetOrigin &explicit_origin = PresetOrigin()); diff --git a/src/libslic3r/PresetBundle.cpp b/src/libslic3r/PresetBundle.cpp index f66a64ad27..3cc426eee6 100644 --- a/src/libslic3r/PresetBundle.cpp +++ b/src/libslic3r/PresetBundle.cpp @@ -1619,7 +1619,8 @@ PresetsConfigSubstitutions PresetBundle::import_presets(std::vector if (ec) BOOST_LOG_TRIVIAL(error) << __FUNCTION__ << " create directory failed: " << ec.message(); //create temp folder //std::string user_default_temp_dir = data_dir() + "/" + PRESET_USER_DIR + "/" + DEFAULT_USER_FOLDER_NAME + "/" + "temp"; - fs::path temp_folder(configs_folder / "temp"); + // Per process, so two instances importing at once do not clear each other's extraction. + fs::path temp_folder(configs_folder / ("temp_" + std::to_string(get_current_pid()))); std::string user_default_temp_dir = temp_folder.make_preferred().string(); if (fs::exists(temp_folder)) fs::remove_all(temp_folder); fs::create_directory(temp_folder, ec); @@ -2241,11 +2242,7 @@ void PresetBundle::remove_user_presets_directory(const std::string preset_folder return; } BOOST_LOG_TRIVIAL(debug) << __FUNCTION__ << boost::format(" enter, delete directory : %1%") % dir_user_presets; - InstanceLock instance_lock(user_presets_lock_path()); - fs::path folder(dir_user_presets); - if (fs::exists(folder)) { - fs::remove_all(folder); - } + remove_directory_tree_locked(fs::path(dir_user_presets)); } void PresetBundle::update_system_preset_setting_ids(std::map>& system_presets) diff --git a/src/libslic3r/Utils.hpp b/src/libslic3r/Utils.hpp index 69551b19af..0a03bbd785 100644 --- a/src/libslic3r/Utils.hpp +++ b/src/libslic3r/Utils.hpp @@ -246,9 +246,6 @@ inline std::error_code write_file_atomically(const std::string &path, const std: // 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. -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 74909d0a98..a8ecef89a2 100644 --- a/src/libslic3r/utils.cpp +++ b/src/libslic3r/utils.cpp @@ -12,6 +12,7 @@ #include #include #include +#include #include #include #include @@ -458,7 +459,7 @@ 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. +// Names a file object by volume and file index. 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); @@ -735,7 +736,8 @@ std::error_code rename_file(const std::string &from, const std::string &to) boost::nowide::remove(aside.c_str()); return {}; } - boost::nowide::rename(aside.c_str(), to.c_str()); + if (boost::nowide::rename(aside.c_str(), to.c_str()) != 0) + BOOST_LOG_TRIVIAL(error) << "Could not put " << to << " back after a failed replace; its previous content is at " << aside; } } return std::make_error_code(static_cast(err)); @@ -819,7 +821,19 @@ std::error_code write_file_atomically(const std::string &path, std::initializer_ // 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"; + // Once per file at warning level, so a mount that never replaces does + // not fill the log, and the loss of the atomic path is still on record. + static std::mutex warned_mutex; + static std::set warned; + bool first; + { + std::lock_guard guard(warned_mutex); + first = warned.insert(path).second; + } + if (first) + BOOST_LOG_TRIVIAL(warning) << "Cannot replace " << path << " (" << ec.message() << "); writing in place, here and for later saves of this file"; + else + BOOST_LOG_TRIVIAL(info) << "Cannot replace " << path << " (" << ec.message() << "); writing in place"; return write_whole_file(path, chunks, binary); } // Crash leftovers of earlier writes into this directory, for the files the @@ -830,27 +844,6 @@ std::error_code write_file_atomically(const std::string &path, std::initializer_ 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 file_identity_of(info); -#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) { // Once an hour per directory: a batch of saves or a load followed by a @@ -910,7 +903,11 @@ size_t remove_stale_temp_files(const boost::filesystem::path &dir, const std::st // 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"; + static std::mutex warned_mutex; + static std::set warned; + std::lock_guard guard(warned_mutex); + if (warned.insert(it->path().string()).second) + 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)) { diff --git a/src/slic3r/GUI/GUI_App.cpp b/src/slic3r/GUI/GUI_App.cpp index 327953ef70..bb12d9a452 100644 --- a/src/slic3r/GUI/GUI_App.cpp +++ b/src/slic3r/GUI/GUI_App.cpp @@ -7624,11 +7624,7 @@ void GUI_App::start_sync_user_preset(bool with_progress_dlg) // Delete the bundle folder and bundle fs::path bundle_folder = fs::path(bundle.path.c_str()).parent_path(); - boost::system::error_code ec; - { - InstanceLock instance_lock(user_presets_lock_path()); - boost::filesystem::remove_all(bundle_folder, ec); - } + remove_directory_tree_locked(bundle_folder); preset_bundle->bundles.WriteLock(); preset_bundle->bundles.m_bundles.erase(bundle.id); diff --git a/tests/libslic3r/test_instance_lock.cpp b/tests/libslic3r/test_instance_lock.cpp index ad24acd0fa..125023cd02 100644 --- a/tests/libslic3r/test_instance_lock.cpp +++ b/tests/libslic3r/test_instance_lock.cpp @@ -109,7 +109,6 @@ TEST_CASE("InstanceLock reopens a lock file that was replaced on disk", "[Instan { ScopedTemporaryFile lock_file(".lock"); const std::string path = lock_file.string(); - ScopedStaticValue interval(InstanceLock::identity_check_interval, 0ms); { InstanceLock lock(path); REQUIRE(lock.locked()); @@ -118,9 +117,7 @@ TEST_CASE("InstanceLock reopens a lock file that was replaced on disk", "[Instan 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.) + // Each outermost guard opens the file afresh, so the deleted path is back. REQUIRE(boost::filesystem::exists(path)); }