diff --git a/src/base/file_lock.cpp b/src/base/file_lock.cpp index c54067f..c0ee43e 100644 --- a/src/base/file_lock.cpp +++ b/src/base/file_lock.cpp @@ -68,23 +68,6 @@ std::wstring construct_mutex_name(const std::string& path) { return NAME_PREFIX + name; } #endif - -#if !defined(_WIN32) -// The max file lock age before it is considered *definitely* stale, in seconds. -// Note: We set this high to accomodate for time zone differences, clock errors, etc. -const time::seconds_t MAX_FILE_LOCK_AGE = 24 * 3600; - -bool file_is_too_old(const std::string& path) { - try { - const auto age = time::seconds_since_epoch() - get_file_info(path).modify_time(); - return age > MAX_FILE_LOCK_AGE; - } catch (const std::exception& e) { - // Note: This is not necessarily an error, since the lock file may no longer exist. - debug::log(debug::DEBUG) << "Unable to determine file age for " << path << ": " << e.what(); - return false; - } -} -#endif // !_WIN32 } // namespace file_lock_t::file_lock_t(const std::string& path, const bool remote_lock) : m_path(path) { @@ -157,115 +140,19 @@ file_lock_t::file_lock_t(const std::string& path, const bool remote_lock) : m_pa // 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. - const int64_t MIN_SLEEP_TIME = 10; - const int64_t MAX_SLEEP_TIME = 50000; - - int64_t total_wait_time = 0; - int64_t sleep_time = MIN_SLEEP_TIME; - int64_t time_until_lock_break = TIME_BETWEEN_LOCK_BREAKS; - - while (total_wait_time < MAX_WAIT_TIME) { - // Try to create the lock file in exclusive mode. - 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_file_handle, pid_str.data(), pid_str.size()); - if (bytes_written != static_cast(pid_str.size())) { - ::unlink(path.c_str()); - ::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; + // Open the lock file (create if necessary). + m_file_handle = ::open(path.c_str(), O_WRONLY | O_CREAT, 0666); + if (m_file_handle != invalid_file_handle()) { + // Use POSIX record locks to lock the entire file in exclusive mode. We acquire the lock in + // blocking mode (i.e. wait for the lock to be available). + struct flock fl = {}; + fl.l_type = F_WRLCK; // Exclusive mode. + fl.l_whence = SEEK_SET; // Lock entire file (l_start = 0, l_len = 0). + if (::fcntl(m_file_handle, F_SETLKW, &fl) == -1) { + debug::log(debug::ERROR) << "Failed to acquire a lock on the lock file " << m_path; + ::close(m_file_handle); + m_file_handle = invalid_file_handle(); } - if (errno != EEXIST) { - debug::log(debug::ERROR) << "Failed to open the lock file (\"" << std::strerror(errno) - << "\")" << path; - break; - } - - // The file was already taken (errno == EEXIST), so try again soon. - - // Time to check if we can break the lock if it's stale. - if (time_until_lock_break < 0) { - auto fd = ::open(path.c_str(), O_RDONLY); - if (fd < 0) { - if (errno != ENOENT) { - debug::log(debug::ERROR) << "Unable to open possibly stale lock for reading: " << path; - break; - } - - // File was removed since we last tried to open it. Try again... - - } else { - // Read the PID from the file. - std::string owner_pid_str; - { - char buf[100]; - size_t bytes_left = sizeof(buf) - 1; - ssize_t bytes_read = 0; - bool finished_reading = false; - while (!finished_reading) { - const auto bytes = ::read(fd, &buf[bytes_read], bytes_left); - if (bytes == 0) { - finished_reading = true; - } else if (bytes < 0) { - debug::log(debug::INFO) << "Unable to read PID from possibly stale lock " << path; - break; - } else { - bytes_left -= static_cast(bytes); - bytes_read += static_cast(bytes); - } - } - close(fd); - if (finished_reading) { - owner_pid_str = std::string(buf, static_cast(bytes_read)); - } - } - - // Convert the file string to a pid_t. - pid_t owner_pid = -1; - try { - owner_pid = static_cast(std::stol(owner_pid_str)); - } catch (...) { - debug::log(debug::INFO) << "Invalid PID for possibly stale lock " << path << ": " - << owner_pid_str; - } - - // If we have the PID of the owner process, we can check if it's still alive. We also check - // if the file is too old to reasonably be held at the moment (this should also work across - // different nodes sharing locks on a network drive, provided that their clocks are not - // wildly out of sync). - const auto owner_process_is_dead = - (owner_pid >= 0 && ::kill(owner_pid, 0) == -1) || file_is_too_old(path); - if (owner_process_is_dead) { - // Ok, the process is dead. Let's remove the lock file and hope for better luck during - // the next iteration. - const auto file_removed = (::unlink(m_path.c_str()) != -1); - if (file_removed) { - debug::log(debug::INFO) << "Removed stale lock " << path << " for PID " << owner_pid; - } else { - debug::log(debug::INFO) - << "Unable to remove stale lock " << path << " for PID " << owner_pid; - } - - // Restart the timer until we try to break the lock again. - time_until_lock_break = TIME_BETWEEN_LOCK_BREAKS; - sleep_time = MIN_SLEEP_TIME; - } - } - } - - // Wait for a small period of time to give other processes a chance to release the lock. - // Note: Handle both short and long blocks by increasing the sleep time for every new try. - std::this_thread::sleep_for(std::chrono::microseconds(sleep_time)); - total_wait_time += sleep_time; - time_until_lock_break -= sleep_time; - sleep_time = std::min(sleep_time * 2, MAX_SLEEP_TIME); } #endif @@ -314,8 +201,8 @@ file_lock_t::~file_lock_t() { } #else if (m_file_handle != invalid_file_handle()) { - // Delete and close the lock file. - ::unlink(m_path.c_str()); + // Close (and thus unlock) the lock file. Note that the lock file will stay around on the file + // system (this is intentional, and any cleanup needs to be handled elsewhere). ::close(m_file_handle); } #endif diff --git a/src/base/file_lock.hpp b/src/base/file_lock.hpp index d6a04a6..600c788 100644 --- a/src/base/file_lock.hpp +++ b/src/base/file_lock.hpp @@ -32,8 +32,11 @@ namespace file { /// 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. +/// Once the lock goes out of scope, the system object is released. +/// +/// After a lock is no longer used by any process, the named system object may or may not be left +/// on the system (depending on the underlying implementation). It is thus up to the user of the +/// lock to guarantee that the necessary cleanup is performed. /// /// Two system object types are supported: /// @@ -76,8 +79,7 @@ public: /// @brief Release the lock. /// - /// This releases the lock that was held. The sycnhronization object (if any) is deleted if this - /// is the last process that holds a lock. + /// This releases the lock that was held. ~file_lock_t(); /// @returns true if the lock was acquired successfully. diff --git a/src/base/file_lock_stresstest.cpp b/src/base/file_lock_stresstest.cpp index 3b679d9..7f852df 100644 --- a/src/base/file_lock_stresstest.cpp +++ b/src/base/file_lock_stresstest.cpp @@ -84,6 +84,9 @@ int main(int argc, const char** argv) { const auto file_lockname = filename + ".lock"; const bool local_locks = (std::string(argv[2]) == "true"); + // Enable error logging. + debug::set_log_level(debug::ERROR); + long last_count = -1; for (int i = 0; i < NUM_LOOPS; ++i) { { diff --git a/src/base/file_lock_test.cpp b/src/base/file_lock_test.cpp index 9f94d5d..d203891 100644 --- a/src/base/file_lock_test.cpp +++ b/src/base/file_lock_test.cpp @@ -33,7 +33,7 @@ TEST_CASE("file_lock_t constructors are behaving as expected") { } TEST_CASE("Remote locks") { - SUBCASE("Acquiring a lock creates and removes a file") { + SUBCASE("Acquiring a lock creates 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); @@ -49,9 +49,6 @@ TEST_CASE("Remote locks") { 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); } SUBCASE("Transfering lock ownership works as expected") { @@ -76,7 +73,7 @@ TEST_CASE("Remote locks") { // 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. + // The 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); @@ -86,14 +83,11 @@ TEST_CASE("Remote locks") { 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") { + SUBCASE("Acquiring a lock 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); diff --git a/test_scripts/run_file_lock_stresstest.sh b/test_scripts/run_file_lock_stresstest.sh index d6739cd..d0fc2c8 100755 --- a/test_scripts/run_file_lock_stresstest.sh +++ b/test_scripts/run_file_lock_stresstest.sh @@ -34,6 +34,9 @@ function run_test { DATA=$(cat "$TESTFILE") rm -f "$TESTFILE" + # Delete the lock file (if any). + rm -f "${TESTFILE}.lock" + # 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."