juli/OrcaSlicer
1
1
Fork 0
mirror of https://github.com/OrcaSlicer/OrcaSlicer.git synced 2026-09-24 09:23:27 +00:00
Code Issues Packages Projects Releases Wiki Activity

Keep a Save From Being Lost to an Open Reader and Cap the Lock Wait

Browse Source 
Windows refuses to replace a file that another process holds open without
FILE_SHARE_DELETE, which is how the C runtime opens files for reading, so
the atomic write could fail against a concurrent reader and drop the save
where the old in-place write had succeeded. Lock the readers that were
still outside the guard, Preset::reload() and the physical printer loader
and writer, and when the rename still fails that way, log it and write in
place as before; losing the save is worse than a torn read. Report the OS
error from a failed write instead of a generic I/O error.

Each leaf guard paid the full two-second wait on its own, so a bulk save
against an instance holding the lock for a long scan stalled once per
preset while holding the preset collection mutex. After a timed-out wait
the same lock file is not waited on again for ten seconds. Read-only scans
never rewrite or delete and many CLI jobs may share one data dir, so they
take no lock and no longer queue behind each other or log about writing.

Take the bundle metadata guard beside the write rather than while the JSON
is built, state the lock-order rule in the header, and give the tests a
temporary file rather than a directory, since the process keeps the lock
file open and a directory holding it cannot be removed on Windows.
This commit is contained in:
Hanif Koh
2026-09-24 16:26:43 +08:00
parent 5181a7fe26
commit 651e48723d
7 changed files with 80 additions and 26 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/libslic3r/InstanceLock.cpp
+10 -2
View File
@@ -25,6 +25,8 @@ struct InstanceLock::Slot
std::unique_ptr<boost::interprocess::file_lock> file_lock;
int depth{0};
bool file_locked{false};
// After a timed-out wait, guards skip waiting until this point.
std::chrono::steady_clock::time_point skip_waiting_until{};
};
InstanceLock::Slot &InstanceLock::slot_for(const std::string &lock_file_path)
@@ -61,7 +63,9 @@ InstanceLock::InstanceLock(const std::string &lock_file_path, std::chrono::milli
m_slot = &slot_for(lock_file_path);
m_slot->mutex.lock();
if (m_slot->depth++ == 0 && m_slot->file_lock) {
const auto deadline = std::chrono::steady_clock::now() + timeout;
const auto now = std::chrono::steady_clock::now();
const bool wait = now >= m_slot->skip_waiting_until;
const auto deadline = now + timeout;
for (;;) {
try {
if (m_slot->file_lock->try_lock()) {
@@ -72,9 +76,13 @@ InstanceLock::InstanceLock(const std::string &lock_file_path, std::chrono::milli
BOOST_LOG_TRIVIAL(warning) << "Cannot lock " << lock_file_path << ": " << e.what();
break;
}
if (! wait)
break;
if (std::chrono::steady_clock::now() >= deadline) {
m_slot->skip_waiting_until = deadline + cooldown;
BOOST_LOG_TRIVIAL(warning) << "Another instance has held " << lock_file_path << " for over "
<< timeout.count() << " ms; writing without the lock";
<< timeout.count() << " ms; proceeding without the lock for the next "
<< cooldown.count() << " ms";
break;
}
std::this_thread::sleep_for(std::chrono::milliseconds(5));
src/libslic3r/InstanceLock.hpp
+15 -4
View File
@@ -8,18 +8,29 @@ 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.
// The OS releases the file lock when its holder exits, so a crashed instance
// never leaves a stale lock behind.
// through an advisory OS file lock on `lock_file_path`, 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.
//
// The lock is best effort: when the lock file cannot be created, or another
// instance still holds it after `timeout`, the guard keeps only the in-process
// mutex and locked() reports false. Writes then proceed unprotected rather than
// letting one hung instance block every other one from saving.
// letting one hung instance block every other one from saving. After such a
// timeout the same lock file is not waited on again for `cooldown`, so a batch
// of saves pays the wait once rather than once per file.
//
// 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.
// That is why the guards sit at the leaf readers and writers, and why a
// batch-level guard around save_user_presets(), which takes the collection
// mutex through delete_preset(), must not be added.
class InstanceLock
{
public:
static constexpr std::chrono::milliseconds default_timeout{2000};
static constexpr std::chrono::milliseconds cooldown{10000};
// An empty path makes the guard a no-op.
explicit InstanceLock(const std::string &lock_file_path, std::chrono::milliseconds timeout = default_timeout);
src/libslic3r/Preset.cpp
+6 -1
View File
@@ -783,6 +783,7 @@ void Preset::reload(Preset const &parent)
std::string reason;
ForwardCompatibilitySubstitutionRule substitution_rule = ForwardCompatibilitySubstitutionRule::Disable;
try {
InstanceLock instance_lock(user_presets_lock_path());
ConfigSubstitutions config_substitutions = config.load_from_json(file, substitution_rule, key_values, reason);
this->config = parent.config;
this->config.apply(std::move(config));
@@ -1718,7 +1719,9 @@ void PresetCollection::load_presets(
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());
// A read-only scan (the CLI) never rewrites or deletes, and many CLI jobs
// may share one data dir, so it does not queue behind the others.
InstanceLock instance_lock(read_only ? std::string() : user_presets_lock_path());
//BBS: change to json format
for (auto &dir_entry : boost::filesystem::directory_iterator(dir))
{
@@ -4218,6 +4221,7 @@ void PhysicalPrinter::update_preset_names_in_config()
void PhysicalPrinter::save(const std::string& file_name_from, const std::string& file_name_to)
{
InstanceLock instance_lock(user_presets_lock_path());
// rename the file
boost::nowide::rename(file_name_from.data(), file_name_to.data());
this->file = file_name_to;
@@ -4334,6 +4338,7 @@ void PhysicalPrinterCollection::load_printers(
std::string errors_cummulative;
// Store the loaded printers into a new vector, otherwise the binary search for already existing presets would be broken.
std::deque<PhysicalPrinter> printers_loaded;
InstanceLock instance_lock(user_presets_lock_path());
//BBS: change to json format
for (auto& dir_entry : boost::filesystem::directory_iterator(dir))
{
src/libslic3r/PresetBundle.cpp
+3 -2
View File
@@ -7931,12 +7931,13 @@ bool BundleMetadata::save_to_json(const std::string& path) const
j["imported_time"] = this->imported_time;
j["updated_time"] = this->updated_time;
InstanceLock instance_lock(user_presets_lock_path());
j["print_presets"] = strip_prefix(this->print_presets);
j["filament_presets"] = strip_prefix(this->filament_presets);
j["printer_presets"] = strip_prefix(this->printer_presets);
if (const std::error_code ec = write_file_atomically(path, j.dump(4))) {
const std::string content = j.dump(4);
InstanceLock instance_lock(user_presets_lock_path());
if (const std::error_code ec = write_file_atomically(path, content)) {
BOOST_LOG_TRIVIAL(error) << "Failed to save bundle metadata to " << path << ": " << ec.message();
return false;
}
src/libslic3r/utils.cpp
+24 -8
View File
@@ -711,21 +711,37 @@ std::error_code rename_file(const std::string &from, const std::string &to)
#endif
}
static std::error_code write_whole_file(const std::string &path, const std::string &content)
{
errno = 0;
boost::nowide::ofstream out(path, std::ios::out | std::ios::trunc);
out << content;
out.close();
if (! out.fail())
return {};
return std::make_error_code(errno != 0 ? static_cast<std::errc>(errno) : std::errc::io_error);
}
std::error_code write_file_atomically(const std::string &path, const std::string &content)
{
const std::string tmp_path = path + "." + std::to_string(get_current_pid()) + ".tmp";
{
boost::nowide::ofstream out(tmp_path, std::ios::out | std::ios::trunc);
out << content;
out.close();
if (out.fail()) {
boost::nowide::remove(tmp_path.c_str());
return std::make_error_code(std::errc::io_error);
}
if (std::error_code ec = write_whole_file(tmp_path, content)) {
boost::nowide::remove(tmp_path.c_str());
return ec;
}
std::error_code ec = rename_file(tmp_path, path);
if (ec)
boost::nowide::remove(tmp_path.c_str());
#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.
// 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.
if (ec == std::errc::permission_denied) {
BOOST_LOG_TRIVIAL(warning) << "Cannot replace " << path << " while another process holds it open; writing in place";
ec = write_whole_file(path, content);
}
#endif
return ec;
}
tests/libslic3r/test_instance_lock.cpp
+19 -8
View File
@@ -20,8 +20,8 @@ using namespace std::chrono_literals;
TEST_CASE("InstanceLock creates its lock file and holds it for the guard's scope", "[InstanceLock]")
{
ScopedTemporaryDir dir;
const std::string path = (dir.path() / "shared.lock").string();
ScopedTemporaryFile lock_file(".lock");
const std::string path = lock_file.string();
{
InstanceLock lock(path);
@@ -37,8 +37,8 @@ TEST_CASE("InstanceLock creates its lock file and holds it for the guard's scope
TEST_CASE("InstanceLock nests within one thread", "[InstanceLock]")
{
ScopedTemporaryDir dir;
const std::string path = (dir.path() / "shared.lock").string();
ScopedTemporaryFile lock_file(".lock");
const std::string path = lock_file.string();
InstanceLock outer(path);
{
@@ -64,8 +64,8 @@ TEST_CASE("InstanceLock is a no-op for an empty path and survives an unwritable
TEST_CASE("InstanceLock serialises the threads of one process", "[InstanceLock]")
{
ScopedTemporaryDir dir;
const std::string path = (dir.path() / "shared.lock").string();
ScopedTemporaryFile lock_file(".lock");
const std::string path = lock_file.string();
std::atomic<bool> holder_ready{false};
std::atomic<bool> holder_released{false};
@@ -93,8 +93,8 @@ TEST_CASE("InstanceLock serialises the threads of one process", "[InstanceLock]"
// Windows through LockFileEx, but spawning a child there is not worth a test.
TEST_CASE("InstanceLock yields to another process and reports it", "[InstanceLock]")
{
ScopedTemporaryDir dir;
const std::string path = (dir.path() / "shared.lock").string();
ScopedTemporaryFile lock_file(".lock");
const std::string path = lock_file.string();
int child_holds[2], child_may_exit[2];
REQUIRE(::pipe(child_holds) == 0);
@@ -122,6 +122,14 @@ TEST_CASE("InstanceLock yields to another process and reports it", "[InstanceLoc
InstanceLock lock(path, 100ms);
locked_while_child_holds = lock.locked();
}
// The timed-out wait starts a cool-down: the next guard does not wait again.
const auto started = std::chrono::steady_clock::now();
bool locked_during_cooldown;
{
InstanceLock lock(path, 5000ms);
locked_during_cooldown = lock.locked();
}
const auto cooldown_wait = std::chrono::steady_clock::now() - started;
REQUIRE(::write(child_may_exit[1], "x", 1) == 1);
int status = 0;
REQUIRE(::waitpid(child, &status, 0) == child);
@@ -129,6 +137,9 @@ TEST_CASE("InstanceLock yields to another process and reports it", "[InstanceLoc
::close(fd);
REQUIRE_FALSE(locked_while_child_holds);
REQUIRE_FALSE(locked_during_cooldown);
REQUIRE(cooldown_wait < 1000ms);
// A guard inside the cool-down still takes the lock when it is free.
InstanceLock lock(path);
REQUIRE(lock.locked());
}
tests/libslic3r/test_utils.cpp
+3 -1
View File
@@ -10,6 +10,7 @@
#include <cctype>
#include <fstream>
#include <string>
#include <system_error>
#ifndef _WIN32
#include <unistd.h> // getuid
@@ -84,7 +85,8 @@ TEST_CASE("write_file_atomically reports a missing directory and writes nothing"
ScopedTemporaryDir dir;
const boost::filesystem::path target = dir.path() / "missing" / "preset.json";
REQUIRE(write_file_atomically(target.string(), "x"));
const std::error_code ec = write_file_atomically(target.string(), "x");
REQUIRE(ec == std::errc::no_such_file_or_directory);
REQUIRE_FALSE(boost::filesystem::exists(target));
}