diff --git a/src/libslic3r/AppConfig.cpp b/src/libslic3r/AppConfig.cpp index b6d0515afe..680583f438 100644 --- a/src/libslic3r/AppConfig.cpp +++ b/src/libslic3r/AppConfig.cpp @@ -987,8 +987,6 @@ void AppConfig::save() BOOST_LOG_TRIVIAL(fatal) << "Calling AppConfig::save() from a worker thread!"; throw CriticalException("Calling AppConfig::save() from a worker thread!"); } - if (std::chrono::steady_clock::now() < m_retry_save_at) - return; // The config is first written to a file with a PID suffix and then moved // to avoid race conditions with multiple instances of Slic3r @@ -1271,8 +1269,6 @@ void AppConfig::save() { if (! is_main_thread_active()) throw CriticalException("Calling AppConfig::save() from a worker thread!"); - if (std::chrono::steady_clock::now() < m_retry_save_at) - return; // The config is first written to a file with a PID suffix and then moved // to avoid race conditions with multiple instances of Slic3r diff --git a/src/libslic3r/AppConfig.hpp b/src/libslic3r/AppConfig.hpp index f3199c272f..5d2ecd9d16 100644 --- a/src/libslic3r/AppConfig.hpp +++ b/src/libslic3r/AppConfig.hpp @@ -131,6 +131,9 @@ public: // Does this config need to be saved? bool dirty() const { return m_dirty; } + // False for ten seconds after a failed write, so the idle handler does not + // repeat a hopeless attempt on every event; an explicit save() always tries. + bool save_due() const { return std::chrono::steady_clock::now() >= m_retry_save_at; } void set_dirty() { m_dirty = true; } @@ -458,8 +461,7 @@ private: bool write_config_file(const std::string &path, std::string body, const std::string &checksum_source); bool m_dirty; - // After a failed write, save() does nothing until this point, so the idle - // handler does not repeat the attempt on every event. + // After a failed write, save_due() is false until this point. std::chrono::steady_clock::time_point m_retry_save_at{}; // Original version found in the ini file before it was overwritten Semver m_orig_version; diff --git a/src/libslic3r/InstanceLock.cpp b/src/libslic3r/InstanceLock.cpp index 9fec7c261c..f030caef19 100644 --- a/src/libslic3r/InstanceLock.cpp +++ b/src/libslic3r/InstanceLock.cpp @@ -28,8 +28,6 @@ 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}; @@ -111,6 +109,7 @@ 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->skip_waiting_until = {}; break; } } catch (const std::exception &e) { diff --git a/src/libslic3r/InstanceLock.hpp b/src/libslic3r/InstanceLock.hpp index 13ef879c73..9d8d3b795a 100644 --- a/src/libslic3r/InstanceLock.hpp +++ b/src/libslic3r/InstanceLock.hpp @@ -41,8 +41,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. Mutable so tests can shorten it. - static inline std::chrono::milliseconds identity_check_interval{5000}; + // 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}; // 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 7042db1cdb..a1773cd805 100644 --- a/src/libslic3r/Preset.cpp +++ b/src/libslic3r/Preset.cpp @@ -1866,6 +1866,7 @@ 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); @@ -1877,6 +1878,7 @@ void PresetCollection::load_presets( ++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); diff --git a/src/libslic3r/utils.cpp b/src/libslic3r/utils.cpp index d2d012c3cf..bd04799854 100644 --- a/src/libslic3r/utils.cpp +++ b/src/libslic3r/utils.cpp @@ -740,7 +740,12 @@ std::error_code rename_file(const std::string &from, const std::string &to) static bool is_writable(const std::string &path) { #ifdef _WIN32 - return ::_waccess(boost::nowide::widen(path).c_str(), 2) == 0; + // _waccess() sees only the read-only attribute; an open for writing sees ACLs too. + 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; + ::CloseHandle(handle); + return true; #else return ::access(path.c_str(), W_OK) == 0; #endif @@ -784,9 +789,9 @@ std::error_code write_file_atomically(const std::string &path, std::initializer_ boost::nowide::remove(tmp_path.c_str()); // A directory that allows writing the file but not creating one beside // it, or a name the longer temporary pushes past the path limit. - const bool only_the_temporary_failed = ec == std::errc::permission_denied || ec == std::errc::no_such_file_or_directory || + const bool only_the_temporary_failed = (target_exists && (ec == std::errc::permission_denied || ec == std::errc::no_such_file_or_directory)) || ec == std::errc::filename_too_long; - if (target_exists && only_the_temporary_failed) { + if (only_the_temporary_failed) { BOOST_LOG_TRIVIAL(warning) << "Cannot create a temporary beside " << path << " (" << ec.message() << "); writing in place"; return write_whole_file(path, chunks, binary); } diff --git a/src/slic3r/GUI/GUI_App.cpp b/src/slic3r/GUI/GUI_App.cpp index 7a91487acf..327953ef70 100644 --- a/src/slic3r/GUI/GUI_App.cpp +++ b/src/slic3r/GUI/GUI_App.cpp @@ -3532,7 +3532,7 @@ bool GUI_App::on_init_inner() update_publish_status(); } - if (m_post_initialized && app_config->dirty()) + if (m_post_initialized && app_config->dirty() && app_config->save_due()) app_config->save(); }); diff --git a/tests/libslic3r/test_instance_lock.cpp b/tests/libslic3r/test_instance_lock.cpp index 36ca30670a..3cd3a0a7d9 100644 --- a/tests/libslic3r/test_instance_lock.cpp +++ b/tests/libslic3r/test_instance_lock.cpp @@ -18,6 +18,15 @@ using namespace Slic3r; using namespace std::chrono_literals; +// Sets a process-wide knob for one test and restores it however the test ends. +template struct ScopedStaticValue +{ + T &ref; + T saved; + ScopedStaticValue(T &ref, T value) : ref(ref), saved(ref) { ref = value; } + ~ScopedStaticValue() { ref = saved; } +}; + TEST_CASE("InstanceLock creates its lock file and holds it for the guard's scope", "[InstanceLock]") { ScopedTemporaryFile lock_file(".lock"); @@ -67,8 +76,7 @@ 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 = 300ms; + ScopedStaticValue cooldown(InstanceLock::cooldown, 300ms); bool before_dir, during_cooldown, after_cooldown; const auto started = std::chrono::steady_clock::now(); @@ -87,7 +95,6 @@ TEST_CASE("InstanceLock retries a lock file it could not open once the cool-down InstanceLock lock(path, 100ms); after_cooldown = lock.locked(); } - InstanceLock::cooldown = saved_cooldown; REQUIRE_FALSE(before_dir); // A loaded runner may take longer than the cool-down to get here; then the @@ -101,8 +108,7 @@ 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; + ScopedStaticValue interval(InstanceLock::identity_check_interval, 0ms); { InstanceLock lock(path); REQUIRE(lock.locked()); @@ -110,7 +116,6 @@ 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