A file system watcher class for on-fly file reloading when it has been modified

Question

I wrote a file sytem watcher class that is used to monitor file modification and then execute user-defined handler. Other file system events like moving or deleting don't need to be monitored. The usage is pretty straightforward:

const auto watcher = fswatcher.on_modify("file.txt", [] () {
        std::cout << "Modified" << std::endl;
});

/* ... */

fswatcher.stop_watcher(watcher);

I came from a C background so C++ is fairly new to me especially its modern features (C++11 and later).

I am a big fan of simplicity so my design principles were the following: KISS, YAGNI, and Do The Simplest Thing That Could Possibly Work.

Also since my application is neither intended to be used in a critical-safety systems nor just needs to be robust I decided to follow the principle „succeed or die“ — the application just terminates if an error occurs. But I am still trying to check for errors when using I/O APIs like inotify or poll to give user meaningful error messages.

---

You might notice a lot of comments in the source code. This is because I use so called the headlines technique. In my opinion it helps to visually split the function's body into several chunks making it easier to read and understand. See also this SoftwareEngineering.SE thread on topic.

I know that this approach have some disadvantages. First of all, the comments could become outdated and, therefore, misleading. Also writting comments takes time as well as maintaining them. But I think that those are better than crazy amount of small functions because:

1. there are less entities to keep track of;
2. you don't lose context while jumping from one such function to another;
3. you don't have possible overhead of function call and parameters passing (yeah, compilers are smart in inlining, but it is a nice bonus to have anyway).

In my opinion splitting the code into several functions makes sense only if they are relatively big and can be reused later.

So what do you think about this commenting style?

---

I also want to port this class to Windows platform later. Do you think that I need to create another level of abstraction and split the FSWatcher code into back-end and front-end: one that works with low-level OS API (external world) and one that manages high-level stuff like watchers and the monitor thread?

In this case I can easily replace the back-end later to whatever I want, but this approach adds an entire another level of abstraction that might make the code harder to understand.

Since the amount of code is really small (176 SLOC) it seems to me that is better not to inroduce back-end part. What do you think?

---

So, basically I am looking for advices on:

1. modern C++ usage;
2. the headlines technique mentioned earlier;
3. Linux API usage (poll, inotify, eventfd);
4. easiness of porting to another platform;
5. code simplicity.

---

include/rosa/fswatcher.h:

// This file contains declaration of the rosa::FSWatcher class.

// This class allows to create a file system watcher on file modification. For
// example:
//
//         std::string file = "vertex.glsl";
//         std::string vertex_shader = read_file(file);
//
//         fswatcher.on_modify(file, [vertex_shader, file] () {
//                 vertex_shader = read_file(file);
//         });
//
// So in this case the shader file "vertex.glsl" will be re-read each time it is
// modified.
//
// The FSWatcher class works ONLY with modification events.
//
// If the file that is being watched is deleted then the correspondig file
// watcher is removed.
//
// The only supported OS for now is Linux.

#ifndef ROSA_FSWATCHER_H
#define ROSA_FSWATCHER_H

// TODO: implement Windows support.
#if !defined(__linux__)
    #error "Only Linux is supported."
#endif

#include <string>
#include <functional>
#include <vector>
#include <thread>
#include <atomic>
#include <poll.h>

namespace rosa {
    class FSWatcher {
    public:
        FSWatcher();
        ~FSWatcher();

        int on_modify(const std::string&, std::function<void()>);
        void stop_watcher(const int);

    private:
        int inotify_fd;
        int self_event_fd;
        std::array<struct pollfd, 2> poll_fds;

        std::thread monitor;
        std::atomic_bool monitor_should_stop;

        typedef struct {
            int id;
            int inotify_wd;
            std::string filename;
            std::function<void()> handler;
        } watcher;

        std::vector<rosa::FSWatcher::watcher> watchers;

        void start_monitoring_thread();
        void stop_monitoring_thread();

        int create_watcher(const std::string& filename, std::function<void()>);
        int create_inotify_watch(const std::string&);

        void handle_self_event();
        void handle_inotify_events();
        void handle_inotify_event(const struct inotify_event *);
    };
}

#endif // ROSA_FSWATCHER_H

src/rosa/fswatcher.cpp:

// This file contains definition of the rosa::FSWatcher class.

// The filesystem watcher is implemented using Linux-specific things:
//
// 1. inotify;
// 2. eventfd;
// 3. poll.
//
// Check out the following man pages for details:
//
// 1. https://man7.org/linux/man-pages/man7/inotify.7.html;
// 2. https://man7.org/linux/man-pages/man2/eventfd.2.html;
// 3. https://man7.org/linux/man-pages/man2/poll.2.html.
//
// No other OSes are supported for now.

#include <cassert>
#include <ranges>
#include <algorithm>
#include <filesystem>
#include <sys/inotify.h>
#include <sys/eventfd.h>
#include <limits.h>
#include <unistd.h>
#include <rosa/utils.h>
#include <rosa/fswatcher.h>

rosa::FSWatcher::FSWatcher()
{
    // Create inotify instance.
    //
    // We need `IN_NONBLOCK` flag since we don't want the `read` call in the
    // `handle_inotify_events` to block the execution.
    inotify_fd = inotify_init1(IN_NONBLOCK);
    if (inotify_fd == -1)
        rosa::utils::die("can't init inotify: {}", strerror(errno));

    // Create eventfd descriptor.
    //
    // Flag `EFD_NONBLOCK` have same purpose as `IN_NONBLOCK` above but in
    // function `handle_self_event`.
    self_event_fd = eventfd(0, EFD_NONBLOCK);
    if (self_event_fd == -1)
        rosa::utils::die("can't create eventfd: {}", strerror(errno));

    // Add the corresponding pollfds.
    poll_fds = {{
        {
            .fd = inotify_fd,
            .events = POLLIN
        },
        {
            .fd = self_event_fd,
            .events = POLLIN
        }
    }};
}

rosa::FSWatcher::~FSWatcher()
{
    // Stop monitoring.
    stop_monitoring_thread();

    // Close the file descriptors.
    close(self_event_fd);
    close(inotify_fd);
}

int
rosa::FSWatcher::on_modify(const std::string& filename, std::function<void()> handler)
{
    // Create new watcher.
    const auto id = rosa::FSWatcher::create_watcher(filename, handler);

    // Start monitoring as soons as we have any watchers to monitor.
    if (watchers.size() == 1)
        start_monitoring_thread();

    // Return the watcher ID so the caller can stop it later using the
    // `stop_watcher` function.
    return id;
}

void
rosa::FSWatcher::stop_watcher(const int id)
{
    // Remove the watcher from list of all the watchers.
    std::erase_if(watchers, [id] (const auto& w) { return w.id == id; });

    // Send some arbitrary data to the `self_event_fd` file descriptor.
    // Actual data doesn't matter since they are ignored by
    // `handle_self_event`.
    //
    // We need to send an event in order to let the monitor thread to
    // terminate if there is no watchers left.
    const uint64_t dummy = 0xDEADBEEF;
    write(self_event_fd, &dummy, sizeof dummy);
}

int
rosa::FSWatcher::create_watcher(const std::string& filename, std::function<void()> handler)
{
    // Add new watcher to the vector.
    watchers.push_back({
        .id = (int) watchers.size(),
        .inotify_wd = rosa::FSWatcher::create_inotify_watch(filename),
        .filename = filename,
        .handler = handler,
    });

    // Return its ID.
    return watchers.back().id;
}

int
rosa::FSWatcher::create_inotify_watch(const std::string& filename)
{
    // Create a watch. We are only interested in file modification, so we
    // mask all other events.
    auto w = inotify_add_watch(inotify_fd, filename.c_str(), IN_MODIFY);
    if (w == -1)
        rosa::utils::die("can't add inotify watch");

    // Return the watch.
    return w;
}

void
rosa::FSWatcher::handle_self_event()
{
    // Dummy read to empty the `self_event_fd` file descriptor to make sure
    // that `poll` don't react on this event again and again.
    uint64_t dummy;
    read(self_event_fd, &dummy, sizeof dummy);
}

void
rosa::FSWatcher::handle_inotify_events()
{
    // This buffer size is enough to read at least one event. See the
    // inotify(7) manpage for details.
    constexpr std::size_t len = sizeof(struct inotify_event) + NAME_MAX + 1;
    char buf[len];

    // Perform a non-blocking read.
    const ssize_t n = read(inotify_fd, buf, sizeof buf);

    // Check for an explicit error. `EAGAIN` and `EWOULDBLOCK` indicate that
    // the call was successfull but there is nothing to read yet.
    if (n == -1 && errno != EAGAIN && errno != EWOULDBLOCK) {
        rosa::utils::warn("some error happened on read call: {}", strerror(errno));
        return;
    }

    // This means that `read` faced EOF.
    //
    // TODO: what does it mean for `read` to face EOF on reading an inotify
    // file descriptor? What should we do in this case?
    if (n <= 0) {
        rosa::utils::warn("read call returned someting <= 0: {}", strerror(errno));
        return;
    }

    // Iterate throught all the event that inotify sent to the buffer.
    for (const char *p = buf; p < buf + n; ) {
        // Extract an event from the buffer, handle it, and then advance
        // the pointer to the next event in raw buffer.
        auto e = (const struct inotify_event *) p;
        rosa::FSWatcher::handle_inotify_event(e);
        p += sizeof *e + e->len;
    }
}

void
rosa::FSWatcher::handle_inotify_event(const struct inotify_event *e)
{
    // Look up the watcher that got an event.
    auto w = std::ranges::find_if(watchers, [e] (const auto& w) {
        return e->wd == w.inotify_wd;
    });

    // We should always find it.
    assert(w != std::end(watchers));

    // I don't know why but deleting the file also causes inotify event even
    // if the watcher was created with IN_MODIFY mask.
    //
    // So we need to check either file still exists or not. And if not then
    // we remove its watcher.
    if (!std::filesystem::exists(w->filename)) {
        inotify_rm_watch(inotify_fd, w->inotify_wd);
        rosa::FSWatcher::stop_watcher(w->id);
        return;
    }

    // If we got an event for this watcher and this is a real modification
    // event then we should call the user-defined handler.
    w->handler();

    // Reinit the inotify watcher.
    //
    // I've spent several hours trying to undestand why inotify stop working
    // after reporting once. Probably the reason is that when you modify the
    // file it is not just being `write` which should send IN_MODIFY event.
    //
    // Some editors may close the file and create a new one with the same
    // name that probably changes the file's inode and, therefore, inotify
    // loses sight of the file.
    //
    // Also it seems that inotify always sets the IN_IGNORED bit in the
    // event's mask.
    //
    // See also this SO thread:
    //
    //     https://stackoverflow.com/q/13409843/8086115
    //
    // TODO: May be there is a better way of doing this. Check it out later.
    inotify_rm_watch(inotify_fd, w->inotify_wd);
    w->inotify_wd = rosa::FSWatcher::create_inotify_watch(w->filename);
}

void
rosa::FSWatcher::start_monitoring_thread()
{
    // Unset the stop-flag.
    monitor_should_stop = false;

    // Create the thread.
    monitor = std::thread([this] () {
        // Poll events while stop-flag is not set and there are any
        // watchers to monitor.
        while (!monitor_should_stop && watchers.size() > 0) {
            // Poll the events.
            const int r = poll(poll_fds.data(), poll_fds.size(), -1 /* no timeout */);
            if (r == -1)
                rosa::utils::warn("some error on poll: {}", strerror(errno));

            // React on inotify event.
            if (poll_fds[0].revents & POLLIN)
                handle_inotify_events();

            // React on our own event.
            if (poll_fds[1].revents & POLLIN)
                handle_self_event();
        }
    });
}

void
rosa::FSWatcher::stop_monitoring_thread()
{
    // Set stop-flag. The thread will check it after getting an event.
    monitor_should_stop = true;

    // We need to send a dummy event in order to let the monitor thread to
    // resume execution after blocking `poll` call.
    const uint64_t dummy = 0xDEADBEEF;
    write(self_event_fd, &dummy, sizeof dummy);

    // Join the thread.
    monitor.join();
}

Answer 1

So what do you think about this commenting style?

It encourages meandering functions, and does not help make sure each "clump" is cohesive. You lose the exit pathway, where you can return when you have the answer as the function is a strong boundary with special semantics; if it's just a clump of lines, that's a goto to jump out of the clump. There's also the "ins" and "outs" of a clump: the "out" is just assignment to some variable, and is not a "initialize when you declare it (and hopefully make it const); rather, it behaves more like an out parameter. The "ins" use values defined earlier in the long function and are not declared cleanly as to which ones this clump actually needs.

Now, the first function of yours I read, the constructor, is not as bad as the cited wiki illustrates. You "headline" assignment to a member and error checking (in the form of postconditions). That's not really a "clump" at all. Instead, the "headline" is just saying the same thing that the code should.

An extreme example is

 // Stop monitoring.
    stop_monitoring_thread();

which is something you should not do. Most of your "headline comments" are really of this nature, and only slightly clarify what the code already says. It would be better to make the code more clearly say what it's doing instead.

// Add new watcher to the vector.
watchers.push_back({

yea, that's what push_back does.

// Return its ID.
return watchers.back().id;

yes, that's what return does. Return the id of the thing that was just pushed... the comment adds nothing.

Back to the constructor: It would be better if members could be set up using member initializers, but I understand that's not always easy due to sequencing and conditional code, and it's not always inefficient to not using proper initializers.

typedef struct {
  ...
  } watcher;

Was that copied/pasted from C code? In C++ you just write struct watcher {...}; as the struct tag is already a full type name.

---

types

You use int a lot, to mean filehandle and maybe other things. You should declare named types to indicate what they are, as opposed to the implementation of the data. Even if you just use a using alias rather than any kind of strong typing mechanism, it's still easier to read and understand, and maintain when a type changes!

This is especially true for your watcher IDs. The token returned from on_modify is a special value to pass to stop_watcher, and you can't just pass any int there. Personally, I've made such tokens encapsulated classes that un-register automatically when destroyed, as opposed to manually passing it to another function.

For a file name, why not use the filesystem library instead of a string?

There's another const std::string& parameter, and I'd like to point out that you should use std::string_view (by value) for such parameters now. It's just as efficient when passing an actual string, but it's also efficient when passing a lexical string literal, as it doesn't have to copy the memory and create a string temporary object!

The destructor shouldn't need to close and probably should not need to stop explicitly. That's the problem with making your members plain int values. You should first tightly encapsulate the "handle" type, so that RAII/RFID. The handle closes when its own destructor is called. Now using that in the larger class that has more than one resource, it's all automatic. The only explicit code in your destructor should be things that coordinate among multiple members.

more housekeeping

Your class's auto-generated copy constructor and assignment operator probably don't work right, as they just copy the int but not duplicate the handle. Remember the Rule of 5: if you need any of the special member functions (and you wrote a destructor) you should supply all of them.

An encapsulated handle type would itself contain logic for copying or prevent it from being copied. Either way, you would not need to write these other functions just as you would not need to write the destructor.

retiring IDs

Your watcher ID is based on the number of items already in the vector. That won't work if you un-watch something! Say you watch three things and they get assigned ids of 0, 1, and 2. Now you revoke 0. That's found in the vector by its id rather than requiring it to be at a specific index value, which is good, but now your vector is shorter. When you add another watched item, it gets id 2 again! Now there are two 2's in the vector.

You should keep a separate variable tracking the next id to be assigned.

Answer 2

They are mostly visual separators for different parts of a single function. ... make clean to the reader which lines of code inside a function are closely related and which are not.

Your first function listed, the constructor, doesn't need such "visual separators" other than a blank line. The closely-related lines come in pairs, one for the real work and one for the error checking.

Having postconditions follow the action is an understandable idiom. The reader, knowing the forms of error action in use, should recognise them as not contributing to the resulting action.

Ideally we'd use a standard visually-distinct construct such as Ensures but that doesn't take a string or automatically process error codes or anything.

The well-understood idiom for preconditions/postconditions is to use a terse if statement with the library-specific handler, without extra braces around it, nice and streamlined.

Some comments on your rosa::utils::die function:
You're repeatedly calling strerror(errno) instead of having the utility do that automatically. There is also the standard error_code and stuff that goes with it, which knows how to look up the error message not only for errno things but for errors associated with other libraries and APIs as well.

namespace util {
std::error_code get_errno()
{
    const auto e = errno;  // POSIX error result
    return std::error_code (e, std::system_category());
}

[[noreturn]]
void croak (std::error_code e, /*sink*/ std::string message)
{
    throw std::system_error (e, std::move(message));
}


} //Ns


rosa::FSWatcher::FSWatcher()
{
    inotify_fd = inotify_init1(IN_NONBLOCK);
    if (inotify_fd == -1)
        util::croak (get_errno(), "can't init inotify");

    self_event_fd = eventfd(0, EFD_NONBLOCK);
    if (self_event_fd == -1)
        util::croak (get_errno(), "can't create eventfd");

    poll_fds = {{
        {
            .fd = inotify_fd,
            .events = POLLIN
        },
        {
            .fd = self_event_fd,
            .events = POLLIN
        }
    }};
}

Only one blank line, plus the idiomatic usage of the pattern

member = do the real work;
if (member not good) raise error;

makes the "close coupling" between these lines clear.
Without that postcondition checking, each line would be a separate thing.

If you had encapsulated the operating system "handle" rather than using a bare int, you could use a member function to check for validity rather than testing against -1. Besides having the destructor write itself, it's even more readable as if (!member.ok()) ....

Answer 3

using string_view

Your comment to my main answer states:

The problem with string_view is that I have to provide a null-terminated [sic] string to Linux API, but string_view doesn't support .c_str() so I have to create a temporary std::string each time.

Here's my code for dealing with that.

/*  Sometmimes we need a nul-terminated const char*, such as for legacy C functions and APIs.
    The string_view is not guaranteed to have a terminating nul after the content, though
    the common uses of passing a lexical string literal and passing a std::string will both
    in-fact have one.  So, provide for the common case and avoid copying the whole string, but
    allow the general case automatically.

    Going past the end is technically undefined behavior.  This implementation can contain
    whatever tricks are needed to prevent the compiler from optimizing out the test.  In real
    CPUs, the only issue would occur when the string_view data is the very last thing in a region
    of committed memory.  The only time we won't have a terminating nul is when the string_view
    is a subset of a larger string, which means there is more data past the end.
*/
class assure_nul_terminated {
    std::string buf;  // if a copy is needed
    string_view val;
public:
    assure_nul_terminated (string_view in)
        {
            const void* hidden = in.cend();
            // prevent the compiler from assuming that *in.cend()=='\0' is undefined and optimizing out the code.
            const char* eptr = static_cast<const char*>(hidden);
            if (*eptr == '\0')  val= in;
            else {
                buf = in;
                val = buf;
            }
        }
    operator const char* () const { return val.data(); }
};