From 9ef14d55ab28ebfd5ec725f09c524269f097c34c Mon Sep 17 00:00:00 2001 From: Adrien DELSALLE Date: Thu, 30 Sep 2021 16:49:49 +0200 Subject: [PATCH] handle files and directories locks handle multiple locks acquired from the same process improve logs add tests refactor use of LockFile forbid locking twice same path from same PID improve windows lockfile to match unix one split Lock and LockFile use byte 21 to set lock status enable reading locked file on windows ensure closing of file descriptor in Lock and LockFile destructors add timeout implementation for unix add testing lock cli for subprocess tests add subprocess tests add --lock-timeout to umamba add clean lock files add a clean lock files flag -l,--locks to umamba clean command add cpp-filesystem pins to enforce correct version on dev mode --- .github/workflows/main.yml | 21 +- environment-dev.yml | 2 +- include/mamba/api/clean.hpp | 1 + include/mamba/core/context.hpp | 2 + include/mamba/core/util.hpp | 69 +++++- src/api/clean.cpp | 54 +++- src/api/configuration.cpp | 10 + src/api/install.cpp | 8 +- src/core/util.cpp | 397 +++++++++++++++++++++++------- src/micromamba/clean.cpp | 6 + src/micromamba/common_options.cpp | 4 + test/CMakeLists.txt | 6 + test/test_lockfile.cpp | 272 ++++++++++++++++++++ test/testing/lock.cpp | 75 ++++++ 14 files changed, 812 insertions(+), 115 deletions(-) create mode 100644 test/test_lockfile.cpp create mode 100644 test/testing/lock.cpp diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index e7f7a1939f..15f3c532d9 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -45,7 +45,7 @@ jobs: run: | conda config --add channels conda-forge conda config --set channel_priority strict - conda install -n base python=$PYTHON_VERSION pip pybind11 libsolv libsodium libarchive "libcurl=7.76.1=*_0" nlohmann_json cpp-filesystem conda cxx-compiler ccache cmake gtest gmock reproc-cpp yaml-cpp + conda install -n base python=$PYTHON_VERSION pip pybind11 libsolv libsodium libarchive "libcurl=7.76.1=*_0" nlohmann_json "cpp-filesystem>=1.5.8" conda cxx-compiler ccache cmake gtest gmock reproc-cpp yaml-cpp env: PYTHON_VERSION: ${{ matrix.python-version }} - name: Install dependencies @@ -147,7 +147,7 @@ jobs: export MAMBA_ROOT_PREFIX=~/mambaroot export MAMBA_EXE=$(pwd)/micromamba . $MAMBA_ROOT_PREFIX/etc/profile.d/mamba.sh - micromamba create -y -p ~/build_env pybind11 libsolv libsodium libarchive "libcurl=7.76.1=*_0" nlohmann_json cxx-compiler ccache cmake gtest gmock cpp-filesystem reproc-cpp yaml-cpp cli11 -c conda-forge + micromamba create -y -p ~/build_env pybind11 libsolv libsodium libarchive "libcurl=7.76.1=*_0" nlohmann_json cxx-compiler ccache cmake gtest gmock "cpp-filesystem>=1.5.8" reproc-cpp yaml-cpp cli11 -c conda-forge env: PYTHON_VERSION: ${{ matrix.python-version }} - name: build tests @@ -169,6 +169,7 @@ jobs: -DBUILD_BINDINGS=OFF \ -DCMAKE_CXX_COMPILER_LAUNCHER=ccache \ -DCMAKE_C_COMPILER_LAUNCHER=ccache + make test_mamba_lock -j2 make test -j2 umamba_tests: @@ -208,7 +209,7 @@ jobs: export MAMBA_ROOT_PREFIX=~/mambaroot export MAMBA_EXE=$(pwd)/micromamba . $MAMBA_ROOT_PREFIX/etc/profile.d/mamba.sh - micromamba create -y -p ~/build_env pybind11 libsolv libsodium libarchive "libcurl=7.76.1=*_0" nlohmann_json cxx-compiler ccache cmake gtest gmock cpp-filesystem reproc-cpp yaml-cpp pyyaml cli11 -c conda-forge + micromamba create -y -p ~/build_env pybind11 libsolv libsodium libarchive "libcurl=7.76.1=*_0" nlohmann_json cxx-compiler ccache cmake gtest gmock "cpp-filesystem>=1.5.8" reproc-cpp yaml-cpp pyyaml cli11 -c conda-forge env: PYTHON_VERSION: ${{ matrix.python-version }} - name: build micromamba @@ -323,7 +324,7 @@ jobs: run: | conda config --add channels conda-forge conda config --set channel_priority strict - conda install -n base -q -y vs2017_win-64 ccache python=$PYTHON_VERSION pip pybind11 libsolv libsodium libarchive "libcurl=7.76.1=*_0" nlohmann_json cpp-filesystem conda cmake gtest gmock ninja reproc-cpp yaml-cpp cli11 + conda install -n base -q -y vs2017_win-64 ccache python=$PYTHON_VERSION pip pybind11 libsolv libsodium libarchive "libcurl=7.76.1=*_0" nlohmann_json "cpp-filesystem>=1.5.8" conda cmake gtest gmock ninja reproc-cpp yaml-cpp cli11 env: PYTHON_VERSION: ${{ matrix.python-version }} - name: Install dependencies @@ -398,7 +399,7 @@ jobs: run: | conda config --add channels conda-forge conda config --set channel_priority strict - conda create -q -y -n mamba-tests vs2017_win-64 ccache python=$PYTHON_VERSION pip pybind11 libsolv libsodium libarchive "libcurl=7.76.1=*_0" nlohmann_json cpp-filesystem conda cmake gtest gmock ninja reproc-cpp yaml-cpp cli11 pytest + conda create -q -y -n mamba-tests vs2017_win-64 ccache python=$PYTHON_VERSION pip pybind11 libsolv libsodium libarchive "libcurl=7.76.1=*_0" nlohmann_json "cpp-filesystem>=1.5.8" conda cmake gtest gmock ninja reproc-cpp yaml-cpp cli11 pytest env: PYTHON_VERSION: ${{ matrix.python-version }} - name: Run C++ tests Windows @@ -412,8 +413,8 @@ jobs: -G "Ninja" ^ -DCMAKE_CXX_COMPILER_LAUNCHER=ccache ^ -DCMAKE_C_COMPILER_LAUNCHER=ccache - - ninja test + ninja test_mamba_lock -j2 + ninja test -j2 umamba_tests_win: runs-on: ${{ matrix.os }} @@ -508,7 +509,7 @@ jobs: run: | conda config --add channels conda-forge conda config --set channel_priority strict - conda create -q -y -n mamba-tests vs2017_win-64 python=$PYTHON_VERSION pip pybind11 libsolv libsodium libarchive "libcurl=7.76.1=*_0" nlohmann_json cpp-filesystem conda cmake gtest gmock ninja reproc-cpp yaml-cpp pyyaml cli11 pytest menuinst + conda create -q -y -n mamba-tests vs2017_win-64 python=$PYTHON_VERSION pip pybind11 libsolv libsodium libarchive "libcurl=7.76.1=*_0" nlohmann_json "cpp-filesystem>=1.5.8" conda cmake gtest gmock ninja reproc-cpp yaml-cpp pyyaml cli11 pytest pytest-lazy-fixture menuinst env: PYTHON_VERSION: ${{ matrix.python-version }} - name: micromamba python based tests @@ -549,7 +550,7 @@ jobs: run: | conda config --add channels conda-forge conda config --set channel_priority strict - conda create -q -y -n mamba-tests vs2017_win-64 python=$PYTHON_VERSION pip pybind11 libsolv libsodium libarchive "libcurl=7.76.1=*_0" nlohmann_json cpp-filesystem conda cmake gtest gmock ninja reproc-cpp yaml-cpp pyyaml cli11 pytest menuinst + conda create -q -y -n mamba-tests vs2017_win-64 python=$PYTHON_VERSION pip pybind11 libsolv libsodium libarchive "libcurl=7.76.1=*_0" nlohmann_json "cpp-filesystem>=1.5.8" conda cmake gtest gmock ninja reproc-cpp yaml-cpp pyyaml cli11 pytest pytest-lazy-fixture menuinst env: PYTHON_VERSION: ${{ matrix.python-version }} - name: micromamba python based tests with pwsh @@ -588,7 +589,7 @@ jobs: run: | conda config --add channels conda-forge conda config --set channel_priority strict - conda create -q -y -n mamba-tests vs2017_win-64 python=$PYTHON_VERSION pip pybind11 libsolv libsodium libarchive "libcurl=7.76.1=*_0" nlohmann_json cpp-filesystem conda cmake gtest gmock ninja reproc-cpp yaml-cpp pyyaml cli11 pytest menuinst + conda create -q -y -n mamba-tests vs2017_win-64 python=$PYTHON_VERSION pip pybind11 libsolv libsodium libarchive "libcurl=7.76.1=*_0" nlohmann_json "cpp-filesystem>=1.5.8" conda cmake gtest gmock ninja reproc-cpp yaml-cpp pyyaml cli11 pytest pytest-lazy-fixture menuinst env: PYTHON_VERSION: ${{ matrix.python-version }} - name: micromamba python based tests diff --git a/environment-dev.yml b/environment-dev.yml index c3b423dea3..5df2c9c710 100644 --- a/environment-dev.yml +++ b/environment-dev.yml @@ -15,7 +15,7 @@ dependencies: - cxx-compiler - gtest - gmock - - cpp-filesystem + - cpp-filesystem >=1.5.8 - reproc-cpp - yaml-cpp - pyyaml diff --git a/include/mamba/api/clean.hpp b/include/mamba/api/clean.hpp index 9cbd761277..f763f9745d 100644 --- a/include/mamba/api/clean.hpp +++ b/include/mamba/api/clean.hpp @@ -14,6 +14,7 @@ namespace mamba const int MAMBA_CLEAN_INDEX = 1 << 1; const int MAMBA_CLEAN_PKGS = 1 << 2; const int MAMBA_CLEAN_TARBALLS = 1 << 3; + const int MAMBA_CLEAN_LOCKS = 1 << 4; void clean(int options); } diff --git a/include/mamba/core/context.hpp b/include/mamba/core/context.hpp index e77c6caa2c..a7c977a093 100644 --- a/include/mamba/core/context.hpp +++ b/include/mamba/core/context.hpp @@ -163,6 +163,8 @@ namespace mamba bool no_rc = false; bool no_env = false; + std::size_t lock_timeout = 0; + // Conda compat bool add_pip_as_python_dependency = true; diff --git a/include/mamba/core/util.hpp b/include/mamba/core/util.hpp index 522a1c0348..463e10fb49 100644 --- a/include/mamba/core/util.hpp +++ b/include/mamba/core/util.hpp @@ -7,6 +7,11 @@ #ifndef MAMBA_CORE_UTIL_HPP #define MAMBA_CORE_UTIL_HPP +#include "mamba/core/mamba_fs.hpp" +#include "mamba/core/output.hpp" + +#include "nlohmann/json.hpp" + #include #include #include @@ -18,11 +23,7 @@ #include #include #include - -#include "nlohmann/json.hpp" - -#include "mamba_fs.hpp" -#include "output.hpp" +#include namespace mamba @@ -132,28 +133,72 @@ namespace mamba fs::path m_path; }; + class Lock; + class LockFile { public: - LockFile(const fs::path& path); + // LockFile(const fs::path& path); + LockFile(const fs::path& path, const std::chrono::seconds& timeout); ~LockFile(); - LockFile(const LockFile&) = delete; - LockFile& operator=(const LockFile&) = delete; - LockFile& operator=(LockFile&&) = default; + int fd() const; - fs::path& path(); - operator fs::path(); +#ifdef _WIN32 + // Using file descriptor on Windows may cause false negative + static bool is_locked(const fs::path& path); +#else + // Opening a new file descriptor on Unix would clear locks + static bool is_locked(int fd); +#endif + static int read_pid(int fd); + + private: + fs::path m_path; + std::chrono::seconds m_timeout; + int m_fd = -1; + + bool set_lock(bool blocking) const; + + int read_pid() const; + bool write_pid(int pid) const; + bool locked() const; + bool lock(int pid, bool blocking) const; + bool unlock() const; + int close_fd(); + void remove() noexcept; + + friend class Lock; + }; + + class Lock + { + public: + Lock(const fs::path& path); + ~Lock(); + + Lock(const Lock&) = delete; + Lock& operator=(const Lock&) = delete; + Lock& operator=(Lock&&) = default; + + bool locked() const; + int fd() const; + fs::path path() const; private: fs::path m_path; - int m_fd; + fs::path m_lock; + std::unique_ptr p_lock_file; + bool m_locked; #if defined(__APPLE__) or defined(__linux__) pid_t m_pid; #else int m_pid; #endif + + bool try_lock(); + bool lock(); }; /************************* diff --git a/src/api/clean.cpp b/src/api/clean.cpp index 8bf2454595..c78f69dace 100644 --- a/src/api/clean.cpp +++ b/src/api/clean.cpp @@ -8,6 +8,7 @@ #include "mamba/api/configuration.hpp" #include "mamba/core/context.hpp" +#include "mamba/core/mamba_fs.hpp" #include "mamba/core/package_cache.hpp" @@ -25,21 +26,22 @@ namespace mamba bool clean_index = options & MAMBA_CLEAN_INDEX; bool clean_pkgs = options & MAMBA_CLEAN_PKGS; bool clean_tarballs = options & MAMBA_CLEAN_TARBALLS; + bool clean_locks = options & MAMBA_CLEAN_LOCKS; - if (!(clean_all || clean_index || clean_pkgs || clean_tarballs)) + if (!(clean_all || clean_index || clean_pkgs || clean_tarballs || clean_locks)) { - std::cout << "Nothing to do." << std::endl; + Console::stream() << "Nothing to do." << std::endl; return; } - Console::print("Collect information.."); + Console::stream() << "Collect information.."; std::vector envs; MultiPackageCache caches(ctx.pkgs_dirs); if (!ctx.dry_run && (clean_index || clean_all)) { - Console::print("Cleaning index cache.."); + Console::stream() << "Cleaning index cache.."; for (auto* pkg_cache : caches.writable_caches()) if (fs::exists(pkg_cache->get_pkgs_dir() / "cache")) @@ -55,6 +57,50 @@ namespace mamba } } + if (!ctx.dry_run && (clean_locks || clean_all)) + { + Console::stream() << "Cleaning lock files.."; + + for (auto* pkg_cache : caches.writable_caches()) + { + if (fs::exists(pkg_cache->get_pkgs_dir())) + for (auto& p : fs::directory_iterator(pkg_cache->get_pkgs_dir())) + { + if (p.exists() && ends_with(p.path().string(), ".lock") + && fs::exists(rstrip(p.path().string(), ".lock"))) + { + try + { + fs::remove(p); + } + catch (...) + { + LOG_WARNING << "Could not clean lock file '" << p.path().string() + << "'"; + } + } + } + + if (fs::exists(pkg_cache->get_pkgs_dir() / "cache")) + for (auto& p : + fs::recursive_directory_iterator(pkg_cache->get_pkgs_dir() / "cache")) + { + if (p.exists() && ends_with(p.path().string(), ".lock")) + { + try + { + fs::remove(p); + } + catch (...) + { + LOG_WARNING << "Could not clean lock file '" << p.path().string() + << "'"; + } + } + } + } + } + if (fs::exists(ctx.root_prefix / "conda-meta")) { envs.push_back(ctx.root_prefix); diff --git a/src/api/configuration.cpp b/src/api/configuration.cpp index 46571c250e..fc1168c646 100644 --- a/src/api/configuration.cpp +++ b/src/api/configuration.cpp @@ -755,6 +755,16 @@ namespace mamba Spend extra time validating package contents. It consists of running cryptographic verifications on channels and packages metadata.)"))); + insert(Configurable("lock_timeout", &ctx.lock_timeout) + .group("Link & Install") + .set_rc_configurable() + .set_env_var_names() + .description("Lock timeout") + .long_description(unindent(R"( + Lock timeout for blocking mode when waiting for another process + to release the path. + Not configurable on Windows: set to 10s by WinAPI)"))); + // Output, Prompt and Flow insert(Configurable("always_yes", &ctx.always_yes) .group("Output, Prompt and Flow Control") diff --git a/src/api/install.cpp b/src/api/install.cpp index 968ef22f26..2ded4ffefe 100644 --- a/src/api/install.cpp +++ b/src/api/install.cpp @@ -339,10 +339,10 @@ namespace mamba std::vector> subdirs; MultiDownloadTarget multi_dl; - std::unique_ptr subdir_download_lock; + std::unique_ptr subdir_download_lock; if (!ctx.offline) { - subdir_download_lock = std::make_unique(cache_dir / "mamba.lock"); + subdir_download_lock = std::make_unique(cache_dir); } std::vector> priorities; @@ -535,7 +535,7 @@ namespace mamba detail::create_target_directory(ctx.target_prefix); } { - LockFile(pkgs_dirs / "mamba.lock"); + Lock pkgs_dirs_lock(pkgs_dirs); trans.execute(prefix_data); } for (auto other_spec : config.at("others_pkg_mgrs_specs") @@ -774,7 +774,7 @@ namespace mamba fs::create_directories(pkgs_dirs); } - LockFile(pkgs_dirs / "mamba.lock"); + Lock pkgs_dirs_lock(pkgs_dirs); std::vector> targets; MultiDownloadTarget multi_dl; diff --git a/src/core/util.cpp b/src/core/util.cpp index eff0ad883b..1c2937fbc5 100644 --- a/src/core/util.cpp +++ b/src/core/util.cpp @@ -4,15 +4,16 @@ // // The full license is in the file LICENSE, distributed with this software. -#include -#include -#include -#include -#include #if defined(__APPLE__) || defined(__linux__) -#include +#include +#include +#include #include +#include +#include + +#include #include #endif @@ -31,6 +32,13 @@ extern "C" #endif +#include +#include +#include +#include +#include +#include + #include "mamba/core/context.hpp" #include "mamba/core/util.hpp" #include "mamba/core/output.hpp" @@ -643,142 +651,363 @@ namespace mamba return prepend(p.c_str(), start, newline); } - int try_lock(int fd, int pid, bool wait) + LockFile::LockFile(const fs::path& path, const std::chrono::seconds& timeout) + : m_path(path) + , m_timeout(timeout) { - // very much inspired by boost file_lock and - // dnf's https://github.com/rpm-software-management/dnf lock.py#L80 -#ifdef _WIN32 - int ret; - if (!wait) + m_fd = open(path.c_str(), O_RDWR | O_CREAT, 0666); + } + + LockFile::~LockFile() + { + if (m_fd > -1) + close_fd(); + } + + void LockFile::remove() noexcept + { + close_fd(); + + std::error_code ec; + LOG_TRACE << "Removing file '" << m_path.string() << "'"; + fs::remove(m_path, ec); + + if (ec) { - ret = _locking(fd, LK_NBLCK, 1 /*lock_file_contents_length()*/); + LOG_ERROR << "Removing lock file '" << m_path.string() << "' failed\n" + << "You may need to remove it later"; } - else + } + + int LockFile::close_fd() + { + int ret = 0; + if (m_fd > -1) { - ret = _locking(fd, LK_LOCK, 1 /*lock_file_contents_length()*/); + ret = close(m_fd); + m_fd = -1; } - if (ret == 0) + return ret; + } + + bool LockFile::unlock() const + { + int ret = 0; + +#ifdef _WIN32 + LOG_TRACE << "Removing lock on '" << m_path.string() << "'"; + _lseek(m_fd, 21, SEEK_SET); + ret = _locking(m_fd, LK_UNLCK, 1 /*lock_file_contents_length()*/); +#endif + return ret == 0; + } + + int LockFile::read_pid(int fd) + { + char pid_buffer[20] = ""; + +#ifdef _WIN32 + _lseek(fd, 0, SEEK_SET); + int read_res = _read(fd, pid_buffer, 20); +#else + lseek(fd, 0, SEEK_SET); + int read_res = read(fd, pid_buffer, 20); +#endif + if (read_res == -1 && errno != EBADF) { - return pid; + LOG_ERROR << "Could not read lockfile (" << strerror(errno) << ")"; + return -1; } - else + + if (strlen(pid_buffer) == 0) + return -1; + + int pid; + try { - return 1; + pid = std::stoi(pid_buffer); } -#else + catch (...) + { + LOG_ERROR << "Could not parse lockfile"; + return -1; + }; + + return pid; + } + + int LockFile::read_pid() const + { + return read_pid(m_fd); + } + +#ifdef _WIN32 + bool LockFile::is_locked(const fs::path& path) + { + // Windows locks are isolated between file descriptor + // We can then test if locked by opening a new one + int fd = _open(path.c_str(), O_RDWR | O_CREAT, 0666); + _lseek(fd, 21L, SEEK_SET); + char buffer[1]; + bool is_locked = _read(fd, buffer, 1) == -1; + _close(fd); + return is_locked; + } +#endif + +#ifndef _WIN32 + bool LockFile::is_locked(int fd) + { + // UNIX/POSIX record locks can't be checked from current process: opening + // then closing a new file descriptor would unset the locks + // 1. compare owner PID written in lockfile with current PID + // 2. call fcntl called with F_GETLK + // -> log an error if fcntl return a different owner PID vs lockfile content + + // Warning: + // If called from the same process as the lockfile one and PID written in + // file is corrupted, the result is a false negative + + // Note: don't use on Windows + // On Windows, if called from the same process, with the lockfile file descriptor + // and PID written in lockfile is corrupted, the result would be a false negative + + int pid = read_pid(fd); + if (pid == getpid()) + return true; + struct flock lock; lock.l_type = F_WRLCK; lock.l_whence = SEEK_SET; - lock.l_start = 0; + lock.l_start = 21; lock.l_len = 1; + fcntl(fd, F_GETLK, &lock); + + if ((lock.l_type == F_UNLCK) && (pid != lock.l_pid)) + LOG_ERROR << "Lock file has wrong owner PID " << pid << ", actual is " << lock.l_pid; + + return lock.l_type != F_UNLCK; + } +#endif + bool LockFile::locked() const + { +#ifdef _WIN32 + return is_locked(m_path); +#else + return is_locked(m_fd); +#endif + } + + bool LockFile::write_pid(int pid) const + { + auto pid_s = std::to_string(pid); +#ifdef _WIN32 + _lseek(m_fd, 0, SEEK_SET); + _chsize_s(m_fd, 0); + return _write(m_fd, pid_s.c_str(), pid_s.size()) > -1; +#else + lseek(m_fd, 0, SEEK_SET); + ftruncate(m_fd, 0); + return write(m_fd, pid_s.c_str(), pid_s.size()) > -1; +#endif + } + +#ifndef _WIN32 + int timedout_set_lock(int fd, struct flock& lock, const std::chrono::seconds& timeout) + { int ret; + std::mutex m; + std::condition_variable cv; - if (wait) - { + std::thread t([&cv, &ret, &fd, &lock]() { ret = fcntl(fd, F_SETLKW, &lock); + cv.notify_one(); + }); + + pthread_t th = t.native_handle(); + t.detach(); - if (ret) + { + std::unique_lock l(m); + if (cv.wait_for(l, timeout) == std::cv_status::timeout) { - LOG_ERROR << "Could not lock file: " << strerror(errno); + pthread_cancel(th); + errno = EINTR; return -1; } } - char pidc[20] = ""; - int read_res = read(fd, pidc, 20); - if (read_res == -1 && errno != EBADF) + return ret; + } +#endif + + bool LockFile::set_lock(bool blocking) const + { + int ret; +#ifdef _WIN32 + _lseek(m_fd, 21, SEEK_SET); + + if (blocking) { - LOG_ERROR << "Could not read old PID from lockfile " << strerror(errno); - return -1; + if (m_timeout.count()) + LOG_WARNING << "Lock timeout can't be set on Windows\n" + << "Timeout value is defined by WinAPI to 10s"; + else // default value is 0 + LOG_DEBUG << "Lock timeout can't be set on Windows\n" + << "Timeout value is defined by WinAPI to 10s"; + ret = _locking(m_fd, LK_LOCK, 1 /*lock_file_contents_length()*/); } + else + ret = _locking(m_fd, LK_NBLCK, 1 /*lock_file_contents_length()*/); +#else + struct flock lock; + lock.l_type = F_WRLCK; + lock.l_whence = SEEK_SET; + lock.l_start = 21; + lock.l_len = 1; - if (strlen(pidc) != 0) + if (blocking) { - int old_pid; - - try - { - old_pid = std::stoi(pidc); - } - catch (...) - { - LOG_ERROR << "Could not parse PID from lock file."; - return -1; - } - - LOG_INFO << "File currently locked by PID: " << old_pid; - if (old_pid == pid) - { - return pid; - } - - // sending `0` with kill will check if the process is still alive - if (kill(old_pid, 0) == -1) - { - lseek(fd, 0, SEEK_SET); - ftruncate(fd, 0); - auto s = std::to_string(pid); - write(fd, s.c_str(), s.size()); - } + if (m_timeout.count()) + ret = timedout_set_lock(m_fd, lock, m_timeout); else - { - return old_pid; - } + ret = fcntl(m_fd, F_SETLKW, &lock); } else + ret = fcntl(m_fd, F_SETLK, &lock); +#endif + return ret == 0; + } + + bool LockFile::lock(int pid, bool blocking) const + { + if (!set_lock(blocking)) { - auto s = std::to_string(pid); - write(fd, s.c_str(), s.size()); + LOG_ERROR << "Could not set lock (" << strerror(errno) << ")"; + return false; } - ret = fcntl(fd, F_SETLK, &lock); - if (ret) + if (!write_pid(pid)) { - LOG_ERROR << "Could not lock file: " << strerror(errno); + LOG_ERROR << "Could not write PID to lockfile (" << strerror(errno) << ")"; + return false; } - return pid; -#endif + return true; } - bool unlock(int fd) + int LockFile::fd() const { + return m_fd; + } + + int Lock::fd() const + { + return p_lock_file->fd(); + } + + bool Lock::lock() + { + return p_lock_file->lock(m_pid, true); + } + + bool Lock::try_lock() + { + int old_pid = p_lock_file->read_pid(); + if (old_pid > 0) + { + if (old_pid == m_pid) + { + LOG_ERROR << "Path already locked by the same PID"; + throw std::logic_error("Lock error."); + } + + LOG_TRACE << "File currently locked by PID " << old_pid; #ifdef _WIN32 - int ret = _locking(fd, LK_UNLCK, 1 /*lock_file_contents_length()*/); + return false; +#else + // sending `0` with kill will check if the process is still alive + if (kill(old_pid, 0) != -1) + return false; #endif - close(fd); - return true; + } + + return p_lock_file->lock(m_pid, false); } - LockFile::LockFile(const fs::path& path) + Lock::Lock(const fs::path& path) : m_path(path) + , m_locked(false) { - LOG_INFO << "Locking " << path; + if (!fs::exists(path)) + { + LOG_DEBUG << "Could not lock non-existing path '" << path.string() << "'"; + return; + } - m_fd = open(path.c_str(), O_RDWR | O_CREAT, 0666); + if (fs::is_directory(path)) + { + LOG_DEBUG << "Locking directory '" << path.string() << "'"; + m_lock = m_path / "mamba.lock"; + } + else + { + LOG_DEBUG << "Locking file '" << path.string() << "'"; + m_lock = m_path.string() + ".lock"; + } + + p_lock_file = std::make_unique( + m_lock, std::chrono::seconds(Context::instance().lock_timeout)); - if (m_fd <= 0) + if (p_lock_file->fd() <= 0) { - LOG_ERROR << "Could not open lockfile " << path; + LOG_DEBUG << "Could not open lockfile '" << m_lock.string() << "'"; } else { m_pid = getpid(); - int ret = try_lock(m_fd, m_pid, false); - if (ret != m_pid) + if (!(m_locked = try_lock())) + { + LOG_WARNING << "Cannot lock '" << m_path.string() << "'" + << "\nWaiting for other mamba process to finish"; + + m_locked = lock(); + } + + if (m_locked) + { + LOG_TRACE << "Lockfile created at '" << m_lock.string() << "'"; + LOG_DEBUG << "Successfully locked"; + } + else { - LOG_WARNING << "Cannot lock file " << path - << "\nWaiting for other mamba process to finish.\n"; - try_lock(m_fd, m_pid, true); + LOG_ERROR << "Lock can't be set at '" << m_path.string() << "'\n" + << "This could be fixed by changing the locks' timeout or " + << "cleaning your environment from previous runs"; + throw std::runtime_error("Lock error. Aborting."); } } } - LockFile::~LockFile() + Lock::~Lock() + { + if (m_locked) + { + LOG_DEBUG << "Unlocking '" << m_path.string() << "'"; + p_lock_file->unlock(); + p_lock_file->remove(); + } + } + + bool Lock::locked() const + { + return m_locked; + } + + fs::path Lock::path() const { - unlock(m_fd); - fs::remove(m_path); + return m_lock; } std::string timestamp(const std::time_t& utc_time) diff --git a/src/micromamba/clean.cpp b/src/micromamba/clean.cpp index 2259845d5c..91fabd41e1 100644 --- a/src/micromamba/clean.cpp +++ b/src/micromamba/clean.cpp @@ -32,12 +32,16 @@ init_clean_parser(CLI::App* subcom) auto& clean_tarballs = config.insert(Configurable("clean_tarballs", false) .group("cli") .description("Remove cached package tarballs")); + auto& clean_locks = config.insert(Configurable("clean_locks", false) + .group("cli") + .description("Remove lock files from caches")); subcom->add_flag("-a,--all", clean_all.set_cli_config(0), clean_all.description()); subcom->add_flag("-i,--index-cache", clean_index.set_cli_config(0), clean_index.description()); subcom->add_flag("-p,--packages", clean_pkgs.set_cli_config(0), clean_pkgs.description()); subcom->add_flag( "-t,--tarballs", clean_tarballs.set_cli_config(0), clean_tarballs.description()); + subcom->add_flag("-l,--locks", clean_locks.set_cli_config(0), clean_locks.description()); } void @@ -57,6 +61,8 @@ set_clean_command(CLI::App* subcom) options = options | MAMBA_CLEAN_PKGS; if (config.at("clean_tarballs").compute().value()) options = options | MAMBA_CLEAN_TARBALLS; + if (config.at("clean_locks").compute().value()) + options = options | MAMBA_CLEAN_LOCKS; clean(options); }); diff --git a/src/micromamba/common_options.cpp b/src/micromamba/common_options.cpp index 78045f96b9..abb44caa26 100644 --- a/src/micromamba/common_options.cpp +++ b/src/micromamba/common_options.cpp @@ -325,6 +325,10 @@ init_install_options(CLI::App* subcom) extra_safety_checks.set_cli_config(0), extra_safety_checks.description()); + auto& lock_timeout = config.at("lock_timeout").get_wrapped(); + subcom->add_option( + "--lock-timeout", lock_timeout.set_cli_config(-1), lock_timeout.description()); + auto& shortcuts = config.at("shortcuts").get_wrapped(); subcom->add_flag( "--shortcuts,!--no-shortcuts", shortcuts.set_cli_config(0), shortcuts.description()); diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 76ea83c402..63306320d0 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -21,6 +21,7 @@ set(TEST_SRCS test_validate.cpp test_virtual_packages.cpp test_env_file_reading.cpp + test_lockfile.cpp ) add_executable(test_mamba ${TEST_SRCS}) @@ -43,3 +44,8 @@ target_link_libraries(test_mamba PUBLIC mamba-static) set_property(TARGET test_mamba PROPERTY CXX_STANDARD 17) add_custom_target(test COMMAND test_mamba DEPENDS test_mamba) + +add_executable(test_mamba_lock testing/lock.cpp) +target_link_libraries(test_mamba_lock PRIVATE GTest::GTest GTest::Main ${CMAKE_THREAD_LIBS_INIT}) +target_link_libraries(test_mamba_lock PUBLIC mamba-static) +set_property(TARGET test_mamba_lock PROPERTY CXX_STANDARD 17) diff --git a/test/test_lockfile.cpp b/test/test_lockfile.cpp new file mode 100644 index 0000000000..c021df679c --- /dev/null +++ b/test/test_lockfile.cpp @@ -0,0 +1,272 @@ +#include + +#include "mamba/core/mamba_fs.hpp" +#include "mamba/core/output.hpp" +#include "mamba/core/util.hpp" + +#include + +#include +#include +#include + +#ifdef _WIN32 +#include + +extern "C" +{ +#include +#include +#include +} +#endif + +namespace mamba +{ + namespace testing + { +#ifdef _WIN32 +#define EXPECT_LOCKED(lock) EXPECT_TRUE(mamba::LockFile::is_locked(lock.path())); +#else +#define EXPECT_LOCKED(lock) EXPECT_TRUE(mamba::LockFile::is_locked(lock.fd())); +#endif + + class LockDirTest : public ::testing::Test + { + protected: + std::unique_ptr p_tempdir; + fs::path tempdir_path; + + LockDirTest() + { + p_tempdir = std::make_unique(); + tempdir_path = p_tempdir->path(); + + mamba::MessageLogger::global_log_severity() = mamba::LogSeverity::kTrace; + } + + ~LockDirTest() + { + mamba::MessageLogger::global_log_severity() = mamba::LogSeverity::kInfo; + } + }; + + TEST_F(LockDirTest, same_pid) + { + { + auto lock = Lock(tempdir_path); + EXPECT_TRUE(lock.locked()); + EXPECT_TRUE(fs::exists(lock.path())); + + EXPECT_THROW(Lock lock2(tempdir_path), std::logic_error); + + // check the first lock is still locked + EXPECT_TRUE(lock.locked()); + EXPECT_TRUE(fs::exists(lock.path())); + } + EXPECT_FALSE(fs::exists(tempdir_path / "mamba.lock")); + } + + TEST_F(LockDirTest, different_pid) + { + std::string lock_cli; + std::string out, err; + std::vector args; + +#ifdef _WIN32 + lock_cli = "test_mamba_lock"; +#else + lock_cli = "./test_mamba_lock"; +#endif + + { + auto lock = Lock(tempdir_path); + EXPECT_TRUE(lock.locked()); + EXPECT_TRUE(fs::exists(lock.path())); + + // Check lock PID + int pid = getpid(); + EXPECT_EQ(mamba::LockFile::read_pid(lock.fd()), pid); + + // Check lock status + EXPECT_LOCKED(lock); + + // Check lock status from another process + args = { lock_cli, "is-locked", lock.path() }; + out.clear(); + err.clear(); + reproc::run( + args, reproc::options{}, reproc::sink::string(out), reproc::sink::string(err)); + + int is_locked = 0; + try + { + is_locked = std::stoi(out); + } + catch (...) + { + } + EXPECT_TRUE(is_locked); + + + // Try to lock from another process + args = { lock_cli, "lock", "--timeout=1", tempdir_path }; + out.clear(); + err.clear(); + reproc::run( + args, reproc::options{}, reproc::sink::string(out), reproc::sink::string(err)); + + bool new_lock_created = true; + try + { + new_lock_created = std::stoi(out); + } + catch (...) + { + } + EXPECT_FALSE(new_lock_created); + + // Check the PID hasn't changed + EXPECT_EQ(mamba::LockFile::read_pid(lock.fd()), pid); + } + + fs::path lock_path = tempdir_path / "mamba.lock"; + EXPECT_FALSE(fs::exists(lock_path)); + + args = { lock_cli, "is-locked", lock_path }; + out.clear(); + err.clear(); + reproc::run( + args, reproc::options{}, reproc::sink::string(out), reproc::sink::string(err)); + + int is_locked = 0; + try + { + is_locked = std::stoi(out); + } + catch (...) + { + } + EXPECT_FALSE(is_locked); + } + + class LockFileTest : public ::testing::Test + { + protected: + std::unique_ptr p_tempfile; + fs::path tempfile_path; + + LockFileTest() + { + p_tempfile = std::make_unique(); + tempfile_path = p_tempfile->path(); + + mamba::MessageLogger::global_log_severity() = mamba::LogSeverity::kTrace; + } + + ~LockFileTest() + { + mamba::MessageLogger::global_log_severity() = mamba::LogSeverity::kInfo; + } + }; + + TEST_F(LockFileTest, same_pid) + { + { + auto lock = Lock(tempfile_path); + EXPECT_TRUE(lock.locked()); + EXPECT_TRUE(fs::exists(lock.path())); + + EXPECT_THROW(Lock lock2(tempfile_path), std::logic_error); + + // check the first lock is still locked + EXPECT_TRUE(lock.locked()); + EXPECT_TRUE(fs::exists(lock.path())); + } + EXPECT_FALSE(fs::exists(tempfile_path.string() + ".lock")); + } + + TEST_F(LockFileTest, different_pid) + { + std::string lock_cli; + std::string out, err; + std::vector args; + +#ifdef _WIN32 + lock_cli = "test_mamba_lock"; +#else + lock_cli = "./test_mamba_lock"; +#endif + { + // Create a lock + auto lock = Lock(tempfile_path); + EXPECT_TRUE(lock.locked()); + EXPECT_TRUE(fs::exists(lock.path())); + + // Check lock PID + int pid = getpid(); + EXPECT_EQ(mamba::LockFile::read_pid(lock.fd()), pid); + + // Check lock status from current PID + EXPECT_LOCKED(lock); + + // Check lock status from another process + args = { lock_cli, "is-locked", lock.path() }; + out.clear(); + err.clear(); + reproc::run( + args, reproc::options{}, reproc::sink::string(out), reproc::sink::string(err)); + + int is_locked = 0; + try + { + is_locked = std::stoi(out); + } + catch (...) + { + } + EXPECT_TRUE(is_locked); + + // Try to lock from another process + args = { lock_cli, "lock", "--timeout=1", tempfile_path }; + out.clear(); + err.clear(); + reproc::run( + args, reproc::options{}, reproc::sink::string(out), reproc::sink::string(err)); + + bool new_lock_created = true; + try + { + new_lock_created = std::stoi(out); + } + catch (...) + { + } + EXPECT_FALSE(new_lock_created); + + // Check the PID hasn't changed + EXPECT_EQ(mamba::LockFile::read_pid(lock.fd()), pid); + } + + fs::path lock_path = tempfile_path.string() + ".lock"; + EXPECT_FALSE(fs::exists(lock_path)); + + args = { lock_cli, "is-locked", lock_path }; + out.clear(); + err.clear(); + reproc::run( + args, reproc::options{}, reproc::sink::string(out), reproc::sink::string(err)); + + int is_locked = 0; + try + { + is_locked = std::stoi(out); + } + catch (...) + { + } + EXPECT_FALSE(is_locked); + } +#undef EXPECT_LOCKED + } +} diff --git a/test/testing/lock.cpp b/test/testing/lock.cpp new file mode 100644 index 0000000000..783a97adab --- /dev/null +++ b/test/testing/lock.cpp @@ -0,0 +1,75 @@ +#include "mamba/core/context.hpp" +#include "mamba/core/mamba_fs.hpp" +#include "mamba/core/util.hpp" +#include "mamba/core/thread_utils.hpp" + +#include + +#if defined(__APPLE__) || defined(__linux__) +#include +#include +#include +#include +#include +#include + +#include +#include +#endif + + +bool +is_locked(const fs::path& path) +{ +#ifdef _WIN32 + return mamba::LockFile::is_locked(path); +#else + // From another process than the lockfile one, we can open/close + // a new file descriptor without risk to clear locks + int fd = open(path.c_str(), O_RDWR | O_CREAT, 0666); + bool is_locked = mamba::LockFile::is_locked(fd); + close(fd); + return is_locked; +#endif +} + +int +main(int argc, char** argv) +{ + CLI::App app{}; + fs::path path; + int timeout = 1; + + CLI::App* lock_com = app.add_subcommand("lock", "Lock a path"); + lock_com->add_option("path", path, "Path to lock"); + lock_com->add_option("-t,--timeout", timeout, "Timeout in seconds"); + lock_com->callback([&]() { + mamba::Context::instance().lock_timeout = timeout; + try + { + mamba::Lock lock(path); + std::cout << lock.locked(); + } + catch (std::runtime_error&) + { + std::cout << 0; + } + }); + + CLI::App* is_locked_com = app.add_subcommand("is-locked", "Check if a path is locked"); + is_locked_com->add_option("path", path, "Path to check"); + is_locked_com->callback([&]() { std::cout << (fs::exists(path) && is_locked(path)); }); + + try + { + CLI11_PARSE(app, argc, argv); + } + catch (const std::exception& e) + { + LOG_ERROR << e.what(); + mamba::set_sig_interrupted(); + return 1; + } + + return 0; +}