Class for locking shared disk directory

Question

I'm writing an application to sync files between two directories. In order to prevent simultaneous access to the shared directory from several computers, I implemented blocking of the shared directory. The class below implements this lock.

The application is built and at first glance works without errors.

But I'm new to writing multithreaded and file system code and I'm not sure I did everything right. Please look at my directory locker class. Is everything right there? Are there any problems? If so, how can they be corrected? Can it be made better?

Update from 12.11: I forgot to write:

1. The application is platform independent and I prefer not to use platform specific functions. It is advisable to use functions from the standard library.
2. The shared directory may be located on a network drive and system functions such as shlock or flock may not be available there.

Header file:

#pragma once
#include <filesystem>
#include <future>

class DirectoryLocker
{
public:
    DirectoryLocker();
    ~DirectoryLocker();

    bool tryToLock(const std::wstring& dirPath);
    void freeLock();
    bool isLockValid() const;

    static constexpr const wchar_t* LockFileName = L".momentasync.lock";
private:
    std::filesystem::path m_pathToLockFile;
    std::atomic<bool> m_stopLockFileRefresh;
    std::future<void> m_lockFileRefreshFuture;
};

CPP file:

#include "DirectoryLocker.h"
#include <chrono>
#include <fstream>


using namespace std::chrono_literals;

const auto LockLifeTime = 15min;
const auto LockRefreshReserve = 1min;

static void writeExpirationTimeToLockFile(FILE* lockFile);
static std::chrono::system_clock::time_point getLockFileExpirationTime(const std::filesystem::path& pathToLockFile);
static std::future<void> runLockFileRefreshTask(const std::atomic<bool>& stopLockFileRefresh, const std::filesystem::path& pathToLockFile);

DirectoryLocker::DirectoryLocker()
{}

DirectoryLocker::~DirectoryLocker()
{
    // According to Effective C++ Item 8: Prevent Exceptions from Leaving Destructors
    // we provide separate function freeLock because freing lock can throw exceptions.
    try {
        if (isLockValid())
            freeLock();
    }
    catch (...)
    {}
}

bool DirectoryLocker::tryToLock(const std::wstring& dirPath)
{
    m_pathToLockFile = dirPath;
    m_pathToLockFile /= LockFileName;

    FILE* fp = std::fopen(m_pathToLockFile.string().c_str(), "wx");
    if (!fp)
    {
        if (getLockFileExpirationTime(m_pathToLockFile) >= std::chrono::system_clock::now())
            return false;

        // Lock file is expired. Remove it and try to create again.
        std::filesystem::remove(m_pathToLockFile);
        fp = std::fopen(m_pathToLockFile.string().c_str(), "wx");
        if (!fp)
            // Someone else created a new lock file before us.
            return false;
    }
    writeExpirationTimeToLockFile(fp);
    std::fclose(fp);

    m_stopLockFileRefresh.store(false, std::memory_order_seq_cst);
    m_lockFileRefreshFuture = runLockFileRefreshTask(m_stopLockFileRefresh, m_pathToLockFile);

    return true;
}

void DirectoryLocker::freeLock()
{
    m_stopLockFileRefresh.store(true, std::memory_order_seq_cst);
    if (m_lockFileRefreshFuture.valid())
        m_lockFileRefreshFuture.wait();
    std::filesystem::remove(m_pathToLockFile);
    std::fstream f;
    f.close();
}

bool DirectoryLocker::isLockValid() const
{
    return m_lockFileRefreshFuture.valid() && m_lockFileRefreshFuture.wait_for(0ms) == std::future_status::timeout;
}

static void writeExpirationTimeToLockFile(FILE* lockFile)
{
    auto expirationTime = std::chrono::system_clock::now() + LockLifeTime;
    std::ostringstream out;
    out << std::chrono::duration_cast<std::chrono::seconds>(expirationTime.time_since_epoch()).count() << std::endl;
    out << expirationTime;
    fputs(out.str().c_str(), lockFile);
}

static void writeExpirationTimeToLockFile(const std::filesystem::path& pathToLockFile)
{
    std::ofstream file(pathToLockFile);
    if (!file)
        throw std::runtime_error("Could not open lock file: " + pathToLockFile.string());

    auto expirationTime = std::chrono::system_clock::now() + LockLifeTime;
    std::ostringstream out;
    file << std::chrono::duration_cast<std::chrono::seconds>(expirationTime.time_since_epoch()).count() << std::endl;
    file << expirationTime;
}

static std::chrono::system_clock::time_point getLockFileExpirationTime(const std::filesystem::path& pathToLockFile)
{
    std::ifstream file(pathToLockFile);
    if (!file)
        throw std::runtime_error("Could not open lock file: " + pathToLockFile.string());

    std::string line;
    if (!std::getline(file, line))
        throw std::runtime_error("Lock file is empty: " + pathToLockFile.string());

    try {
        long long sec = std::stol(line);
        auto timePoint = std::chrono::time_point<std::chrono::system_clock>(std::chrono::seconds(sec));
        auto diffToFuture = timePoint - std::chrono::system_clock::now();
        if (diffToFuture > LockLifeTime || sec <= 0)
        {
            std::ostringstream out;
            out << "Invalid expiration time in the lock file." << std::endl;
            out << "Expiration time: " << timePoint << std::endl;
            out << "Expiration time in seconds sicne epoch: " << sec << std::endl;
            out << "First line of the lock file: " << line << std::endl;
            throw std::runtime_error(out.str());
        }

        return timePoint;
    }
    catch (const std::invalid_argument&) {
        throw std::runtime_error("Invalid first line in the lock file.\nFirst line: \"" + line + "\".\nLock file: " + pathToLockFile.string());
    }
    catch (const std::out_of_range&) {
        throw std::runtime_error("Number in the first line of the lock file is out of range.\nFirst line: \"" + line + "\".\nLock file: " + pathToLockFile.string());
    }
}

static std::future<void> runLockFileRefreshTask(const std::atomic<bool>& stopLockFileRefresh, const std::filesystem::path& pathToLockFile)
{
    return std::async(std::launch::async, [&stopLockFileRefresh, pathToLockFile]() {
        while (!stopLockFileRefresh.load(std::memory_order_seq_cst))
        {
            std::this_thread::sleep_for(std::chrono::milliseconds(500));
            if (std::chrono::system_clock::now() > getLockFileExpirationTime(pathToLockFile) - LockRefreshReserve)
            {
                writeExpirationTimeToLockFile(pathToLockFile);
            }
        }
    });
}

Usage example:

DirectorySyncronizer::SyncResult DirectorySyncronizer::syncronizeFiles(
    const std::wstring& sharedPath,
    const std::atomic<bool>& stopSync,
    const std::function<void(const std::wstring&)>& localSyncProgress,
    const std::function<void(const std::wstring&)>& sharedSyncProgress)
{
    try {
        DirectoryLocker dirLocker;
        if (!dirLocker.tryToLock(sharedPath))
            return { false, "The shared directory already locked by another computer."};

        // Imitation of file copying
        for (int i = 0; i < 360; ++i)
        {
            if (stopSync.load(std::memory_order_seq_cst))
            {
                dirLocker.freeLock();
                return { false, "Syncronization was stopped by the user." };
            }

            if (!dirLocker.isLockValid())
                break;

            std::this_thread::sleep_for(500ms);

            localSyncProgress(std::format(L"File updated: {}", i));
            sharedSyncProgress(std::format(L"File updated: {}", i));
        }

        dirLocker.freeLock();
        return { true, "" };
    }
    catch (const std::exception& error)
    {
        return { false, error.what() };
    }
}

Answer 1

This is not safe

Let's consider two threads that each call tryToLock() at the same time, and there is an expired lockfile on the disk:

FILE* fp = std::fopen(m_pathToLockFile.string().c_str(), "wx");
if (!fp)
{
    if (getLockFileExpirationTime(m_pathToLockFile) >= std::chrono::system_clock::now())
        return false;

    // Lock file is expired. Remove it and try to create again.

Both threads are now right at that comment. Now consider that one thread runs ahead of the other, and calls:

    std::filesystem::remove(m_pathToLockFile);
    fp = std::fopen(m_pathToLockFile.string().c_str(), "wx");

That works. But now the second thread catches up and it too executes:

    std::filesystem::remove(m_pathToLockFile);
    fp = std::fopen(m_pathToLockFile.string().c_str(), "wx");

So it will remove the file opened by the first thread. But that means the second fopen() will succeed. Both threads will now think they have the lock.

Either you should use the file locking mechanisms of your operating system, like for example flock() (which might not be portable and/or not work on shared drives), or rely on rename() being atomic, like J_H mentioned in his answer.

It is not robust

There are lots of things that can go wrong that you are not handling in your code. What if someone renames the directory you are in while you are holding the lock? What if daylight saving time kicks in? What if the two processes run on different computers, and the two computers have the clock set to a different time? What if the lock file could be opened but writing to it fails because the disk is full?

Part of these issues can be solved by adding some more checking to the code, but some problems are unsolvable without using a completely different way to do locking. Again, the operating system might have better functions available to do proper file locking. It also depends on the protocol used to share the drives. For example, flock() works on NFS, but other protocols might behave differently, and/or have different ways to lock files.

Improve the interface

Use the [[nodiscard]] attribute for tryToLock(); this will catch mistakes like calling tryToLock() but not using its return value.

I would also suggest that you let the constructor take the path to the directory, and rename tryToLock() to try_lock(), and freeLock() to unlock(), and perhaps add a lock() that waits indefinitely until it can get the lock. That way it has the same interface as std::mutex, which also means you can use std::lock_guard, std::try_lock() and other utilities that work on lockable objects.

Answer 2

spec

at first glance works without errors.

I'm willing to believe that's true.
You've not made it easy to verify it at second glance.

What I'm looking for is a concise specification of intended behavior. This seems to be a #reinventing-the-wheel of shlock or flock, which writes a pid to a lock file and respects the lock for as long as that pid is still alive.

Curiously, you focus on timestamp rather than pid, and redundantly record timestamp within the file even though stat() mtime already provides it. There could be good reasons for these design decisions, but we don't see them appearing in OP comments or associated documentation.

---

atomic file operations

Sometimes the filesystem resides on a distant server, such as an NFS server.

Or a local attacker is creating annoying ephemeral directory entries.

In either case, it is helpful to rely on the atomic semantics of rename. Randomly create a new name which other processes could not know about, e.g. "foo.lock.998a81". Write necessary data to that file, such as timestamp. Now rename("foo.lock.998a81", "foo.lock"). If racing processes attempt this, exactly one of them wins, installing a lock file with properly formatted contents. Of course, if PID is not part of the contents, that will make adjudicating "winner" a bit harder.

For background, rsync uses this technique to good effect, ensuring that racing rsync processes always produce the right result.

---

FS locks

Sometimes the directory of interest is on a local filesystem, where the local kernel can coordinate processes. POSIX offers good facilities, such as lockf. We might choose to ignore such well-tested facilities, but only after writing at least a one-line comment explaining why we considered and rejected the option. You owe that much to future maintainers who with bug report in hand will wade into this code and will try to make sense of what came before and what they should do next.

---

This codebase appears to achieve some of its proposed goals.

On a system where flock was available, I would prefer that over a call to this library.