From 64c6f73d8b324129933296730a1bc9e9cf12610c Mon Sep 17 00:00:00 2001 From: Christopher League Date: Sat, 20 Apr 2024 19:55:25 -0600 Subject: [PATCH 1/2] Lockfile will warn periodically if waiting more than 5s Fixes #232 --- src/fileutils.cpp | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/src/fileutils.cpp b/src/fileutils.cpp index dfc203210..59479c9ac 100644 --- a/src/fileutils.cpp +++ b/src/fileutils.cpp @@ -28,6 +28,7 @@ #include "fileutils.hpp" #include +#include #if defined(HAVE_CXX_FILESYSTEM) && HAVE_CXX_FILESYSTEM #include @@ -193,6 +194,9 @@ std::string get_dirname(std::string filename) { potentially write the PID into the lock file to help with tracking whether the process died. */ LockFile::LockFile(std::string lockfile) : _lockfile(lockfile) { + time_t start = time(NULL), elapsed = 0; + int busycount = 0; + int busyinterval = 1 << 17; while( true ) { _fd = open(_lockfile.c_str(), O_CREAT, 600); flock(_fd, LOCK_EX); @@ -202,9 +206,22 @@ LockFile::LockFile(std::string lockfile) : _lockfile(lockfile) { // Compare inodes if( fd_stat.st_ino == lockfile_stat.st_ino ) { // Got the lock + if(elapsed > 0) { + std::cerr << "NOTE: acquired " << lockfile + << std::endl; + } break; } + busycount++; close(_fd); + if(busycount % busyinterval == 0) { + elapsed = time(NULL) - start; + if(elapsed >= 5) { + std::cerr << "NOTE: waiting " << elapsed + << "s for " << lockfile << std::endl; + busyinterval = busycount; + } + } } } @@ -212,3 +229,25 @@ LockFile::~LockFile() { unlink(_lockfile.c_str()); flock(_fd, LOCK_UN); } + + +#ifdef FILEUTILS_LOCKFILE_TEST +// This is a little test program for LockFile, to simulate +// a leftover lock file. It should look something like this: + +// Initiating lock... +// NOTE: waiting 5s for ./myfile.lock +// NOTE: waiting 9s for ./myfile.lock +// NOTE: waiting 17s for ./myfile.lock +// NOTE: waiting 34s for ./myfile.lock +// [Delete the file from another terminal] +// NOTE: acquired ./myfile.lock +// Lock acquired... exiting +int main() { + int fd = open("./myfile.lock", O_CREAT, 600); + std::cerr << "Initiating lock..." << std::endl; + LockFile("./myfile.lock"); + std::cerr << "Lock acquired... exiting" << std::endl; + return 0; +} +#endif From ec07ffa08a35765b53346c74f470fede8e27a656 Mon Sep 17 00:00:00 2001 From: Christopher League Date: Mon, 15 Jul 2024 12:53:50 -0400 Subject: [PATCH 2/2] Write PID to Lockfile --- src/fileutils.cpp | 107 ++++++++++++++++++++++++++++------------------ src/fileutils.hpp | 1 + 2 files changed, 67 insertions(+), 41 deletions(-) diff --git a/src/fileutils.cpp b/src/fileutils.cpp index 59479c9ac..0793827b9 100644 --- a/src/fileutils.cpp +++ b/src/fileutils.cpp @@ -29,6 +29,7 @@ #include "fileutils.hpp" #include #include +#include // strerror #if defined(HAVE_CXX_FILESYSTEM) && HAVE_CXX_FILESYSTEM #include @@ -188,52 +189,74 @@ std::string get_dirname(std::string filename) { #endif } -/* NOTE: In case of abnormal exit (such as segmentation fault or other signal), - the lock file will not be removed, and the next attempt to lock might busy- - wait until the file is manually deleted. If this is a common issue, we could - potentially write the PID into the lock file to help with tracking whether - the process died. */ LockFile::LockFile(std::string lockfile) : _lockfile(lockfile) { - time_t start = time(NULL), elapsed = 0; - int busycount = 0; - int busyinterval = 1 << 17; - while( true ) { - _fd = open(_lockfile.c_str(), O_CREAT, 600); - flock(_fd, LOCK_EX); - struct stat fd_stat, lockfile_stat; - fstat(_fd, &fd_stat); - stat(_lockfile.c_str(), &lockfile_stat); - // Compare inodes - if( fd_stat.st_ino == lockfile_stat.st_ino ) { - // Got the lock - if(elapsed > 0) { - std::cerr << "NOTE: acquired " << lockfile - << std::endl; - } - break; - } - busycount++; - close(_fd); - if(busycount % busyinterval == 0) { - elapsed = time(NULL) - start; - if(elapsed >= 5) { - std::cerr << "NOTE: waiting " << elapsed - << "s for " << lockfile << std::endl; - busyinterval = busycount; - } - } - } + time_t start = time(NULL), elapsed = 0; + long busycount = 0; + long busyinterval = 1 << 17; + pid_t pid = getpid(); + while( true ) { + _fd = open(_lockfile.c_str(), O_CREAT|O_RDWR, S_IRUSR|S_IWUSR); + if( _fd == -1 ) { + if(busycount == 0) { + std::cerr << "ERROR: could not create " << lockfile << ": " + << strerror(errno) << std::endl; + } + } + else { + if( flock(_fd, LOCK_EX | LOCK_NB) == 0 ) { + struct stat fd_stat, lockfile_stat; + fstat(_fd, &fd_stat); + stat(_lockfile.c_str(), &lockfile_stat); + if( fd_stat.st_ino == lockfile_stat.st_ino ) { + // We acquired the lock. Announce it only if we announced waiting. + if(elapsed > 0) { + std::cerr << "NOTE: acquired " << lockfile << std::endl; + } + // Exit the busy loop + break; + } + // Locking succeeded, but inodes were different so try again. + flock(_fd, LOCK_UN); + } + close(_fd); + } + busycount++; + if(busycount % busyinterval == 0) { + elapsed = time(NULL) - start; + if(elapsed >= 5) { + std::cerr << "NOTE: waiting " << elapsed + << "s for " << lockfile << std::endl; + busyinterval = busycount; + } + } + } + // We have the lock, so try writing PID. + if(ftruncate(_fd, 0) == -1) { + std::cerr << "WARNING: could not truncate " << lockfile << ": " + << strerror(errno) << std::endl; + } + else { + std::ostringstream ss; + ss << pid << '\n'; + std::string buf = ss.str(); + if(write(_fd, buf.c_str(), buf.size()) != (ssize_t)buf.size()) { + std::cerr << "WARNING: could not write to " << lockfile << ": " + << strerror(errno) << std::endl; + } + } } LockFile::~LockFile() { - unlink(_lockfile.c_str()); - flock(_fd, LOCK_UN); + unlink(_lockfile.c_str()); + flock(_fd, LOCK_UN); + close(_fd); } #ifdef FILEUTILS_LOCKFILE_TEST -// This is a little test program for LockFile, to simulate -// a leftover lock file. It should look something like this: +// This is a little test program for LockFile. Run it in two terminals +// to simulate race conditions, or kill one to leave a a leftover lock file. +// Output should look something like this: // Initiating lock... // NOTE: waiting 5s for ./myfile.lock @@ -243,11 +266,13 @@ LockFile::~LockFile() { // [Delete the file from another terminal] // NOTE: acquired ./myfile.lock // Lock acquired... exiting + int main() { - int fd = open("./myfile.lock", O_CREAT, 600); std::cerr << "Initiating lock..." << std::endl; - LockFile("./myfile.lock"); - std::cerr << "Lock acquired... exiting" << std::endl; + LockFile lock("./myfile.lock"); + std::cerr << "Lock acquired, 'working' for 15s..." << std::endl; + sleep(15); + std::cerr << "Work finished, releasing lock..." << std::endl; return 0; } #endif diff --git a/src/fileutils.hpp b/src/fileutils.hpp index 7664a1cfb..0017c3966 100644 --- a/src/fileutils.hpp +++ b/src/fileutils.hpp @@ -36,6 +36,7 @@ #include // For getpid #include // For getpwuid #include +#include #if defined(__APPLE__) && __APPLE__ #include