lock_file: Refactor the code

* Don't depend on config_t in lock_file_t
* Fixes for long path names
* Add unit and stress tests for local locks
* Rename config to BUILDCACHE_REMOTE_LOCKS (default = false)

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.

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)

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)

src/base/lock_file.cpp

@@ -19,10 +19,10 @@
 
 #include <base/debug_utils.hpp>
 #include <base/file_utils.hpp>
+#include <base/hasher.hpp>
 #include <base/lock_file.hpp>
 #include <base/time_utils.hpp>
 #include <base/unicode_utils.hpp>
-#include <config/configuration.hpp>
 
 #include <cstdint>
 
@@ -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<ssize_t>(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
 }

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<void*>(-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;
 };
 

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] << " <filename>\n";
+  if (argc != 3) {
+    std::cout << "Usage: " << argv[0] << " <filename> <local_locks>\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);

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);
+    }
+  }
 }

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<cache_entry_t, file::lock_file_t> 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();

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

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();

src/main.cpp

@@ -196,6 +196,8 @@ std::unique_ptr<bcache::program_wrapper_t> 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";
     }

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