diff --git a/doc/configuration.md b/doc/configuration.md index 7473b7d..d279b14 100644 --- a/doc/configuration.md +++ b/doc/configuration.md @@ -30,7 +30,7 @@ The following options control the behavior of BuildCache: | `BUILDCACHE_ACCURACY` | `accuracy` | Caching accuracy (see below) | DEFAULT | | `BUILDCACHE_READ_ONLY` | `read_only` | Only read and use the cache without updating it | false | | `BUILDCACHE_IMPERSONATE` | `impersonate` | Explicitly set the executable to wrap | None | -| `BUILDCACHE_LOCAL_LOCKS` | `local_locks` | Use faster lockfile implementation, only safe if the cache is not on a fileshare | false | +| `BUILDCACHE_REMOTE_LOCKS` | `remote_locks` | Use a (potentially slower) file locking mechanism that is safe if the local cache is on a fileshare | false | Note: Currently, only the TI C6x back end supports the `cache_link_commands` option. diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index a8ea510..b446a54 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -84,7 +84,7 @@ if(DOXYGEN_FOUND AND NOT (${CMAKE_VERSION} VERSION_LESS "3.9.0")) set(DOXYGEN_OUTPUT_DIRECTORY doc) set(DOXYGEN_GENERATE_HTML YES) set(DOXYGEN_GENERATE_LATEX NO) - set(DOXYGEN_EXCLUDE third_party) + set(DOXYGEN_EXCLUDE_PATTERNS "*/third_party/*") set(DOXYGEN_EXTRACT_PRIVATE YES) set(DOXYGEN_EXTRACT_STATIC YES) set(DOXYGEN_QUIET YES) diff --git a/src/base/CMakeLists.txt b/src/base/CMakeLists.txt index d94a360..8c1d0b5 100644 --- a/src/base/CMakeLists.txt +++ b/src/base/CMakeLists.txt @@ -58,7 +58,7 @@ if(HAS_HMAC) endif() add_library(base ${BASE_SRC}) -target_link_libraries(base xxhash lz4 zstd config) +target_link_libraries(base xxhash lz4 zstd) if(WIN32 OR MINGW) target_link_libraries(base userenv) elseif(OPENSSL_FOUND) diff --git a/src/base/lock_file.cpp b/src/base/lock_file.cpp index 318cef0..9c9ff70 100644 --- a/src/base/lock_file.cpp +++ b/src/base/lock_file.cpp @@ -19,10 +19,10 @@ #include #include +#include #include #include #include -#include #include @@ -50,6 +50,24 @@ namespace bcache { namespace file { namespace { +#if defined(_WIN32) +std::wstring construct_mutex_name(const std::string& path) { + const std::wstring NAME_PREFIX = L"Global\\buildcache_"; // Place in the Global namespace. + + // Use the full path, without backslashes, as the name. + auto name = utf8_to_ucs2(path); + std::replace(name.begin(), name.end(), L'\\', L'_'); + if ((NAME_PREFIX.size() + name.size()) >= MAX_PATH) { + // If the name does not fit within MAX_PATH, use a hash instead. + hasher_t hasher; + hasher.update(ucs2_to_utf8(name)); + name = utf8_to_ucs2(hasher.final().as_string()); + } + + return NAME_PREFIX + name; +} +#endif + #if !defined(_WIN32) // The max lock file age before it is considered *definitely* stale, in seconds. // Note: We set this high to accomodate for time zone differences, clock errors, etc. @@ -68,49 +86,33 @@ bool file_is_too_old(const std::string& path) { #endif // !_WIN32 } // namespace -lock_file_t::handle_t lock_file_t::invalid_handle() { +lock_file_t::lock_file_t(const std::string& path, const bool remote_lock) : m_path(path) { #if defined(_WIN32) - if (config::local_locks()) { - return nullptr; - } else { - return INVALID_HANDLE_VALUE; - } -#else - return -1; -#endif -} + // Time values are in milliseconds. + const DWORD MAX_WAIT_TIME = 10000; // We'll fail if the lock can't be acquired in 10s. -lock_file_t::lock_file_t(const std::string& path) : m_path(path) { -#if defined(_WIN32) - if (config::local_locks()) { - // Time values are in milliseconds. - const DWORD MAX_WAIT_TIME = 10000; // We'll fail if the lock can't be acquired in 10s. + // For Windows, we can use local, named mutexes instead of file locks, if we're allowed to. + if (!remote_lock) { + // Construct a unique mutex name that identifies the given path. + const auto name = construct_mutex_name(path); - // Just convert the path into a mutex name. - auto path_w = utf8_to_ucs2(path); - // Object names may not contain backslashes. - std::replace(path_w.begin(), path_w.end(), L'\\', L'_'); - // Place in the Global namespace, so the mutex will work for any instance of buildcache on the - // local machine. - const auto name = L"Global\\" + path_w; - // Get a handle to a named mutex. The object may be created now for our call, or previously for - // another instance of buildcache - it doesn't make a difference. - m_handle = CreateMutexW(nullptr, FALSE, name.c_str()); - if (m_handle == invalid_handle()) { + // Open the named mutex (create it if it does not already exist). + m_mutex_handle = CreateMutexW(nullptr, FALSE, name.c_str()); + if (m_mutex_handle == invalid_mutex_handle()) { return; } - const DWORD status = WaitForSingleObject(m_handle, MAX_WAIT_TIME); + + // Acquire the mutex. // WAIT_OBJECT_0 and WAIT_ABANDONED both indicate the lock has been acquired. WAIT_ABANDONED // additionally indicates that the previous thread owning the lock terminated without properly // releasing it. + const auto status = WaitForSingleObject(m_mutex_handle, MAX_WAIT_TIME); if (!(status == WAIT_OBJECT_0 || status == WAIT_ABANDONED)) { - CloseHandle(m_handle); - m_handle = invalid_handle(); + return; } } else { // Time values are in milliseconds. - const DWORD MAX_WAIT_TIME = 10000; // We'll fail if the lock can't be acquired in 10s. - const DWORD MIN_SLEEP_TIME = 0; // We start with 0, which is essentially just a yield. + const DWORD MIN_SLEEP_TIME = 0; // We start with 0, which is essentially just a yield. const DWORD MAX_SLEEP_TIME = 50; DWORD total_wait_time = 0; @@ -118,14 +120,14 @@ lock_file_t::lock_file_t(const std::string& path) : m_path(path) { while (total_wait_time < MAX_WAIT_TIME) { // Try to create the lock file in exclusive mode. - m_handle = CreateFileW(utf8_to_ucs2(path).c_str(), - GENERIC_READ | GENERIC_WRITE, - 0, // No sharing => exclusive access. - nullptr, - CREATE_ALWAYS, - FILE_ATTRIBUTE_NORMAL | FILE_FLAG_DELETE_ON_CLOSE, - nullptr); - if (m_handle != INVALID_HANDLE_VALUE) { + m_file_handle = CreateFileW(utf8_to_ucs2(path).c_str(), + GENERIC_READ | GENERIC_WRITE, + 0, // No sharing => exclusive access. + nullptr, + CREATE_ALWAYS, + FILE_ATTRIBUTE_NORMAL | FILE_FLAG_DELETE_ON_CLOSE, + nullptr); + if (m_file_handle != invalid_file_handle()) { // We got the lock! break; } else { @@ -150,6 +152,10 @@ lock_file_t::lock_file_t(const std::string& path) : m_path(path) { } } #else + // For POSIX, there's no need to use local locks (e.g. named semaphores), since the performance + // benefit is negligable, and there are many inherent problems with that solution. + (void)remote_lock; + // Time values are in microseconds. const int64_t MAX_WAIT_TIME = 10000000; // We'll fail if the lock can't be acquired in 10s. const int64_t TIME_BETWEEN_LOCK_BREAKS = 100000; // We try to break the lock every 100ms. @@ -162,15 +168,15 @@ lock_file_t::lock_file_t(const std::string& path) : m_path(path) { while (total_wait_time < MAX_WAIT_TIME) { // Try to create the lock file in exclusive mode. - m_handle = ::open(path.c_str(), O_WRONLY | O_CREAT | O_EXCL, 0666); - if (m_handle >= 0) { + m_file_handle = ::open(path.c_str(), O_WRONLY | O_CREAT | O_EXCL, 0666); + if (m_file_handle != invalid_file_handle()) { // We got the lock! Write our PID to the file. const auto pid_str = std::to_string(getpid()); - const auto bytes_written = ::write(m_handle, pid_str.data(), pid_str.size()); + const auto bytes_written = ::write(m_file_handle, pid_str.data(), pid_str.size()); if (bytes_written != static_cast(pid_str.size())) { ::unlink(path.c_str()); - ::close(m_handle); - m_handle = invalid_handle(); + ::close(m_file_handle); + m_file_handle = invalid_file_handle(); debug::log(debug::ERROR) << "Failed to write our PID to the lock file " << path; } break; @@ -261,24 +267,31 @@ lock_file_t::lock_file_t(const std::string& path) : m_path(path) { #endif // Did we get the lock? - m_has_lock = (m_handle != invalid_handle()); + m_has_lock = + (m_file_handle != invalid_file_handle()) || (m_mutex_handle != invalid_mutex_handle()); if (m_has_lock) { debug::log(debug::DEBUG) << "Locked " << path; } } lock_file_t::lock_file_t(lock_file_t&& other) noexcept - : m_path(std::move(other.m_path)), m_handle(other.m_handle), m_has_lock(other.m_has_lock) { - other.m_handle = invalid_handle(); + : m_path(std::move(other.m_path)), + m_file_handle(other.m_file_handle), + m_mutex_handle(other.m_mutex_handle), + m_has_lock(other.m_has_lock) { + other.m_file_handle = invalid_file_handle(); + other.m_mutex_handle = invalid_mutex_handle(); other.m_has_lock = false; } lock_file_t& lock_file_t::operator=(lock_file_t&& other) noexcept { m_path = std::move(other.m_path); - m_handle = other.m_handle; + m_file_handle = other.m_file_handle; + m_mutex_handle = other.m_mutex_handle; m_has_lock = other.m_has_lock; - other.m_handle = invalid_handle(); + other.m_file_handle = invalid_file_handle(); + other.m_mutex_handle = invalid_mutex_handle(); other.m_has_lock = false; return *this; @@ -286,24 +299,21 @@ lock_file_t& lock_file_t::operator=(lock_file_t&& other) noexcept { lock_file_t::~lock_file_t() { #if defined(_WIN32) - if (config::local_locks()) { - if (m_handle) { - if (m_has_lock) { - ReleaseMutex(m_handle); - } - CloseHandle(m_handle); - } - } else { - if (m_handle != INVALID_HANDLE_VALUE) { - // Close the lock file. This also deletes the file due to FILE_FLAG_DELETE_ON_CLOSE. - CloseHandle(m_handle); + if (m_mutex_handle != invalid_mutex_handle()) { + if (m_has_lock) { + ReleaseMutex(m_mutex_handle); } + CloseHandle(m_mutex_handle); + } + if (m_file_handle != invalid_file_handle()) { + // Close the file handle. This also deletes the file due to FILE_FLAG_DELETE_ON_CLOSE. + CloseHandle(m_file_handle); } #else - if (m_handle >= 0) { - // 1) Delete and close the lock file. + if (m_file_handle != invalid_file_handle()) { + // Delete and close the lock file. ::unlink(m_path.c_str()); - ::close(m_handle); + ::close(m_file_handle); } #endif } diff --git a/src/base/lock_file.hpp b/src/base/lock_file.hpp index 9bcefbe..6f95508 100644 --- a/src/base/lock_file.hpp +++ b/src/base/lock_file.hpp @@ -24,20 +24,49 @@ namespace bcache { namespace file { -/// @brief A scoped exclusive lock file class. +/// @brief A scoped exclusive global lock class. /// -/// When the locked file object is created, a lock file is created (if necessary) and acquired. Once -/// the object goes out of scope, it releases the lock and deletes the file if this is the last -/// process that releases the lock. +/// This lock class is intended to be used for granular synchronization of multiple processes that +/// need to access a specific part of a file system, such as a single file or a folder. Locks are +/// expected to be held for a very short time period (typically only for a fraction of a second), +/// during operations such as file renames or writes. +/// +/// When the lock is created, a global named system object is created (if necessary) and acquired. +/// Once the lock goes out of scope, the system object is released. If this is the last process that +/// releases the lock, the system object is deleted. +/// +/// Two system object types are supported: +/// +/// \par Remote locks +/// A remote lock is implemented as a lock file that will safely synchronize access to any file +/// system, even a remote network share. +/// +/// \par Local locks +/// A local lock may be implemented as a system synchronization object (e.g. a named mutex or +/// semaphore), that is only guaranteed to be safe for synchronizing file system access on a local +/// system. +/// +/// \par +/// The advantage of a local lock is that on some systems it is faster than a remote lock. +/// +/// \note +/// Do not mix the use of remote locks and local locks for synchronizing access to a single file +/// system, since the locks may live in different namespaces and thus are unaware of each other. +/// +/// \note +/// On some systems, local locks are implemented as remote locks. class lock_file_t { public: - /// @brief Create an empty (unlocked) file lock object. + /// @brief Create an empty (unlocked) lock object. lock_file_t() { } - /// @brief Acquire a lock using the specified file path. - /// @param path The full path to the lock file (will be created). - explicit lock_file_t(const std::string& path); + /// @brief Acquire a lock for the specified file path. + /// @param path The full path to the lock file (will be created). The path should be a location on + /// the file system to which the process requires access synchronization. + /// @param remote_lock Require the implementation to use a locking mechanism that can synchronize + /// file system access across several OS instances (e.g. use this for network shares). + explicit lock_file_t(const std::string& path, const bool remote_lock); // Support move semantics. lock_file_t(lock_file_t&& other) noexcept; @@ -47,8 +76,8 @@ public: /// @brief Release the lock. /// - /// This releases the lock that was held on the file (if any), and deletes the lock file (if this - /// is the last process that holds a lock on the file). + /// This releases the lock that was held. The sycnhronization object (if any) is deleted if this + /// is the last process that holds a lock. ~lock_file_t(); /// @returns true if the lock was acquired successfully. @@ -58,17 +87,34 @@ public: private: #if defined(_WIN32) - // WIN32 API file handle type: HANDLE (i.e. void*). - using handle_t = void*; + // WIN32 API HANDLE type is void*. + using file_handle_t = void*; + using mutex_handle_t = void*; #else - // POSIX file descriptor type: int. - using handle_t = int; + // POSIX file descriptor type is int. + using file_handle_t = int; + using mutex_handle_t = void*; // dummy #endif - static handle_t invalid_handle(); + static file_handle_t invalid_file_handle() { +#if defined(_WIN32) + return reinterpret_cast(-1); // INVALID_HANDLE_VALUE +#else + return -1; +#endif + } + + static mutex_handle_t invalid_mutex_handle() { +#if defined(_WIN32) + return nullptr; +#else + return nullptr; +#endif + } std::string m_path; - handle_t m_handle = invalid_handle(); + file_handle_t m_file_handle = invalid_file_handle(); + mutex_handle_t m_mutex_handle = invalid_mutex_handle(); bool m_has_lock = false; }; diff --git a/src/base/lock_file_stresstest.cpp b/src/base/lock_file_stresstest.cpp index edffdb1..2825381 100644 --- a/src/base/lock_file_stresstest.cpp +++ b/src/base/lock_file_stresstest.cpp @@ -74,19 +74,21 @@ long read_number_from_file(const std::string& path) { } // namespace int main(int argc, const char** argv) { - // The program takes one argument: The name of a file that is to be updated in a locked fashion. - if (argc != 2) { - std::cout << "Usage: " << argv[0] << " \n"; + if (argc != 3) { + std::cout << "Usage: " << argv[0] << " \n"; + std::cout << " filename The name of the file to be updated in a locked fashion\n"; + std::cout << " local_locks Set this to \"true\" to allow local locks\n"; exit(0); } - std::string filename(argv[1]); - std::string lock_filename = filename + ".lock"; + const std::string filename(argv[1]); + const auto lock_filename = filename + ".lock"; + const bool local_locks = (std::string(argv[2]) == "true"); long last_count = -1; for (int i = 0; i < NUM_LOOPS; ++i) { { // Acquire a lock, which should guarantee us exclusive access to the data file. - file::lock_file_t lock(lock_filename); + file::lock_file_t lock(lock_filename, local_locks); if (!lock.has_lock()) { std::cerr << "*** Error: Unable to acquire lock: " << lock_filename << std::endl; exit(1); diff --git a/src/base/lock_file_test.cpp b/src/base/lock_file_test.cpp index e7ba1b8..de78d25 100644 --- a/src/base/lock_file_test.cpp +++ b/src/base/lock_file_test.cpp @@ -32,61 +32,106 @@ TEST_CASE("lock_file_t constructors are behaving as expected") { } } -TEST_CASE("Acquiring a lock creates and removes a file") { - // Get a temporary file name. - file::tmp_file_t tmp_file(file::get_temp_dir(), ".lock"); - REQUIRE_GT(tmp_file.path().size(), 0); +TEST_CASE("Remote locks") { + SUBCASE("Acquiring a lock creates and removes a file") { + // Get a temporary file name. + file::tmp_file_t tmp_file(file::get_temp_dir(), ".lock"); + REQUIRE_GT(tmp_file.path().size(), 0); - // The file should not yet exist. - CHECK_EQ(file::file_exists(tmp_file.path()), false); - - // Aquire a lock in a scope. - { - file::lock_file_t lock(tmp_file.path()); - - // We should now have the lock, and the file should exist. - CHECK_EQ(lock.has_lock(), true); - CHECK_EQ(file::file_exists(tmp_file.path()), true); - } - - // The lock file should no longer exist after the lock has gone out of scope. - CHECK_EQ(file::file_exists(tmp_file.path()), false); -} - -TEST_CASE("Transfering lock ownership works as expected") { - // Get a temporary file name. - file::tmp_file_t tmp_file(file::get_temp_dir(), ".lock"); - REQUIRE_GT(tmp_file.path().size(), 0); - - // The file should not yet exist. - CHECK_EQ(file::file_exists(tmp_file.path()), false); - - { - file::lock_file_t lock; + // The file should not yet exist. + CHECK_EQ(file::file_exists(tmp_file.path()), false); // Aquire a lock in a scope. { - file::lock_file_t child_lock(tmp_file.path()); + file::lock_file_t lock(tmp_file.path(), true); // We should now have the lock, and the file should exist. - CHECK_EQ(child_lock.has_lock(), true); - CHECK_EQ(file::file_exists(tmp_file.path()), true); - - // Move the lock object to the parent scope. - lock = std::move(child_lock); - - // The lock in the child scope should no longer have the lock, but the file should still - // exist. - CHECK_EQ(child_lock.has_lock(), false); + CHECK_EQ(lock.has_lock(), true); CHECK_EQ(file::file_exists(tmp_file.path()), true); } - // The lock in the parent scope should now have the lock, and the file should still exist. - // We should now have the lock, and the file should exist. - CHECK_EQ(lock.has_lock(), true); - CHECK_EQ(file::file_exists(tmp_file.path()), true); + // The lock file should no longer exist after the lock has gone out of scope. + CHECK_EQ(file::file_exists(tmp_file.path()), false); } - // The lock file should no longer exist after the lock has gone out of scope. - CHECK_EQ(file::file_exists(tmp_file.path()), false); + SUBCASE("Transfering lock ownership works as expected") { + // Get a temporary file name. + file::tmp_file_t tmp_file(file::get_temp_dir(), ".lock"); + REQUIRE_GT(tmp_file.path().size(), 0); + + // The file should not yet exist. + CHECK_EQ(file::file_exists(tmp_file.path()), false); + + { + file::lock_file_t lock; + + // Aquire a lock in a scope. + { + file::lock_file_t child_lock(tmp_file.path(), true); + + // We should now have the lock, and the file should exist. + CHECK_EQ(child_lock.has_lock(), true); + CHECK_EQ(file::file_exists(tmp_file.path()), true); + + // Move the lock object to the parent scope. + lock = std::move(child_lock); + + // TThe ownership of the lock should now have moved, and the file should still exist. + CHECK_EQ(child_lock.has_lock(), false); + CHECK_EQ(lock.has_lock(), true); + CHECK_EQ(file::file_exists(tmp_file.path()), true); + } + + // The lock in the parent scope should now have the lock, and the file should still exist. + CHECK_EQ(lock.has_lock(), true); + CHECK_EQ(file::file_exists(tmp_file.path()), true); + } + + // The lock file should no longer exist after the lock has gone out of scope. + CHECK_EQ(file::file_exists(tmp_file.path()), false); + } +} + +TEST_CASE("Local locks") { + SUBCASE("Acquiring a lock creates and removes a file") { + // Get a temporary file name. + file::tmp_file_t tmp_file(file::get_temp_dir(), ".lock"); + REQUIRE_GT(tmp_file.path().size(), 0); + + // Repeatedly aquire and release a lock in a loop scope. + for (int i = 0; i < 10; ++i) { + file::lock_file_t lock(tmp_file.path(), false); + + // We should now have the lock. + CHECK_EQ(lock.has_lock(), true); + } + } + + SUBCASE("Transfering lock ownership works as expected") { + // Get a temporary file name. + file::tmp_file_t tmp_file(file::get_temp_dir(), ".lock"); + REQUIRE_GT(tmp_file.path().size(), 0); + + { + file::lock_file_t lock; + + // Aquire a lock in a scope. + { + file::lock_file_t child_lock(tmp_file.path(), false); + + // We should now have the lock. + CHECK_EQ(child_lock.has_lock(), true); + + // Move the lock object to the parent scope. + lock = std::move(child_lock); + + // The ownership of the lock should now have moved. + CHECK_EQ(child_lock.has_lock(), false); + CHECK_EQ(lock.has_lock(), true); + } + + // The lock in the parent scope should now have the lock. + CHECK_EQ(lock.has_lock(), true); + } + } } diff --git a/src/cache/local_cache.cpp b/src/cache/local_cache.cpp index b3548c1..53829b6 100644 --- a/src/cache/local_cache.cpp +++ b/src/cache/local_cache.cpp @@ -160,7 +160,7 @@ void local_cache_t::clear() { for (const auto& dir : dirs) { try { // We acquire an exclusive lock for the cache entry before deleting it. - file::lock_file_t lock(cache_entry_lock_file_path(dir.path())); + file::lock_file_t lock(cache_entry_lock_file_path(dir.path()), config::remote_locks()); if (lock.has_lock()) { file::remove_dir(dir.path()); } @@ -192,7 +192,7 @@ void local_cache_t::show_stats() { visitedDirs.insert(firstLevelDirPath); const auto stats_path = file::append_path(firstLevelDirPath, STATS_FILE_NAME); cache_stats_t stats; - file::lock_file_t lock{stats_path + LOCK_FILE_SUFFIX}; + file::lock_file_t lock{stats_path + LOCK_FILE_SUFFIX, config::remote_locks()}; if (!lock.has_lock()) { debug::log(debug::DEBUG) << "failed to lock stats, skipping"; return; @@ -235,7 +235,7 @@ void local_cache_t::zero_stats() { for (const auto& dir : first_level_dirs) { try { const auto stats_path = file::append_path(dir, STATS_FILE_NAME); - file::lock_file_t lock{stats_path + LOCK_FILE_SUFFIX}; + file::lock_file_t lock{stats_path + LOCK_FILE_SUFFIX, config::remote_locks()}; if (lock.has_lock()) { file::remove_file(stats_path); } @@ -256,7 +256,7 @@ void local_cache_t::add(const hasher_t::hash_t& hash, { // Acquire a scoped exclusive lock for the cache entry. - file::lock_file_t lock(cache_entry_lock_file_path(cache_entry_path)); + file::lock_file_t lock(cache_entry_lock_file_path(cache_entry_path), config::remote_locks()); if (!lock.has_lock()) { throw std::runtime_error("Unable to acquire a cache entry lock for writing."); } @@ -305,7 +305,7 @@ std::pair local_cache_t::lookup(const hasher_t } // Acquire a scoped lock for the cache entry. - file::lock_file_t lock(cache_entry_lock_file_path(cache_entry_path)); + file::lock_file_t lock(cache_entry_lock_file_path(cache_entry_path), config::remote_locks()); if (!lock.has_lock()) { throw std::runtime_error("Unable to acquire a cache entry lock for reading."); } @@ -332,7 +332,7 @@ bool local_cache_t::update_stats(const hasher_t::hash_t& hash, const cache_stats file::create_dir_with_parents(cache_subdir); } const auto stats_file_path = file::append_path(cache_subdir, STATS_FILE_NAME); - file::lock_file_t lock(stats_file_path + LOCK_FILE_SUFFIX); + file::lock_file_t lock(stats_file_path + LOCK_FILE_SUFFIX, config::remote_locks()); if (!lock.has_lock()) { debug::log(debug::INFO) << "Failed to lock stats, skipping update"; return false; @@ -401,7 +401,7 @@ void local_cache_t::perform_housekeeping() { << dir.access_time() << ", " << dir.size() << " bytes)"; // We acquire a scoped lock for the cache entry before deleting it. - file::lock_file_t lock(cache_entry_lock_file_path(dir.path())); + file::lock_file_t lock(cache_entry_lock_file_path(dir.path()), config::remote_locks()); if (lock.has_lock()) { file::remove_dir(dir.path()); total_size -= dir.size(); diff --git a/src/config/configuration.cpp b/src/config/configuration.cpp index ed57026..ccefa1b 100644 --- a/src/config/configuration.cpp +++ b/src/config/configuration.cpp @@ -58,7 +58,7 @@ int32_t s_compress_level = -1; bool s_perf = false; bool s_disable = false; bool s_read_only = false; -bool s_local_locks = false; +bool s_remote_locks = false; config::cache_accuracy_t s_accuracy = config::cache_accuracy_t::DEFAULT; config::compress_format_t s_compress_format = config::compress_format_t::DEFAULT; @@ -300,11 +300,11 @@ void load_from_file(const std::string& file_name) { } } - // Get "local_locks". + // Get "remote_locks". { - const auto* node = cJSON_GetObjectItemCaseSensitive(root, "local_locks"); + const auto* node = cJSON_GetObjectItemCaseSensitive(root, "remote_locks"); if (cJSON_IsBool(node)) { - s_local_locks = cJSON_IsTrue(node); + s_remote_locks = cJSON_IsTrue(node); } } @@ -542,9 +542,9 @@ void init() { // Get the local lock flag from the environment. { - const env_var_t local_locks_env("BUILDCACHE_LOCAL_LOCKS"); - if (local_locks_env) { - s_local_locks = local_locks_env.as_bool(); + const env_var_t remote_locks_env("BUILDCACHE_REMOTE_LOCKS"); + if (remote_locks_env) { + s_remote_locks = remote_locks_env.as_bool(); } } } catch (...) { @@ -642,8 +642,8 @@ bool read_only() { return s_read_only; } -bool local_locks() { - return s_local_locks; +bool remote_locks() { + return s_remote_locks; } } // namespace config diff --git a/src/config/configuration.hpp b/src/config/configuration.hpp index d0405cc..6250665 100644 --- a/src/config/configuration.hpp +++ b/src/config/configuration.hpp @@ -120,8 +120,8 @@ bool disable(); /// @returns true if the readonly mode is enabled. bool read_only(); -/// @returns true if lock_file_t may use implementation optimized for sharing on local machine only. -bool local_locks(); +/// @returns true if BuildCache must use file locks that are safe for remote file systems. +bool remote_locks(); /// @returns the cache accuracy. cache_accuracy_t accuracy(); diff --git a/src/main.cpp b/src/main.cpp index 1ff136a..efd2573 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -196,6 +196,8 @@ std::unique_ptr find_suitable_wrapper( << (bcache::config::disable() ? "true" : "false") << "\n"; std::cout << " BUILDCACHE_READ_ONLY: " << (bcache::config::read_only() ? "true" : "false") << "\n"; + std::cout << " BUILDCACHE_REMOTE_LOCKS: " + << (bcache::config::remote_locks() ? "true" : "false") << "\n"; std::cout << " BUILDCACHE_ACCURACY: " << to_string(bcache::config::accuracy()) << "\n"; } diff --git a/test_scripts/run_lock_file_stresstest.sh b/test_scripts/run_lock_file_stresstest.sh index 8d8ed09..bcb0042 100755 --- a/test_scripts/run_lock_file_stresstest.sh +++ b/test_scripts/run_lock_file_stresstest.sh @@ -2,43 +2,66 @@ # Note: This script is expected to be run from the build folder. -TESTFILE=/tmp/bc_lock_file_stresstest_data-$$ +total_success=true -rm -f "$TESTFILE" +function run_test { + TESTFILE=/tmp/bc_lock_file_stresstest_data-$$ -# Run four instances of the stresstest in parallel. -echo "Starting four concurrent processes..." -pids="" -for i in {1..4}; do - base/lock_file_stresstest "$TESTFILE" & - pids+=" $!" -done + rm -f "$TESTFILE" -# Wait for all processes to finish. -got_error=false -for p in $pids ; do - if ! wait $p ; then - got_error=true + test_type="network share safe locks" + if [[ "$1" = "true" ]] ; then + test_type="allow local locks" fi -done -# Get the result and delete the file. -DATA=$(cat "$TESTFILE") -rm -f "$TESTFILE" + # Run four instances of the stresstest in parallel. + echo "Starting four concurrent processes (${test_type})..." + pids="" + for i in {1..4}; do + base/lock_file_stresstest "$TESTFILE" $1 & + pids+=" $!" + done -# Did we have an error exit status from any of the processes? -if $got_error ; then - echo "*** FAIL: At least one of the processes failed." - exit 1 -fi + # Wait for all processes to finish. + got_error=false + for p in $pids ; do + if ! wait $p ; then + got_error=true + fi + done -# Check the data file contents. -EXPECTED_DATA="4000" -if [[ "${DATA}" = "${EXPECTED_DATA}" ]] ; then - echo "The test passed!" + # Get the result and delete the file. + DATA=$(cat "$TESTFILE") + rm -f "$TESTFILE" + + # Did we have an error exit status from any of the processes? + if $got_error ; then + echo "*** FAIL: At least one of the processes failed." + exit 1 + fi + + # Check the data file contents. + EXPECTED_DATA="4000" + if [[ "${DATA}" = "${EXPECTED_DATA}" ]] ; then + echo "The test passed!" + else + echo "*** FAIL: The count should be ${EXPECTED_DATA}, but is ${DATA}." + total_success=false + fi +} + +# Without local locks. +run_test false + +# With local locks. +run_test true + +# Check the test result. +if $total_success ; then + echo "All tests passed!" exit 0 fi -echo "*** FAIL: The count should be ${EXPECTED_DATA}, but is ${DATA}." +echo "*** FAIL: At least one of the tests failed." exit 1