File Watching Stability Fixes (#64)

- [use std::atomic<bool> in the filewatcher tests to avoid potential d…](af81af9)
- use `constexpr` constants to clean up test cases
- wait and poll in these test case to allow early exit
- increase some max waits in the test cases to be safer, though they can finish earlier due to polling
- [make linux file watcher use std::atomic<bool> for the running var…](cb48547)

Fixes #52
Fixes #32

Author: jamylak

include/filewatcher/linux_filewatcher.h

@@ -1,6 +1,7 @@
 #ifndef LINUX_FILEWATCHER_H
 #define LINUX_FILEWATCHER_H
 #include "filewatcher.h"
+#include <atomic>
 #include <string>
 #include <thread>
 
@@ -18,6 +19,6 @@ class LinuxFileWatcher : public FileWatcher {
     std::string filename; // Relative filname we watch
     int fd = -1;          // File descriptor
     int wd = -1;          // Watch descriptor
-    bool running = true;
+    std::atomic<bool> running{true};
 };
 #endif

src/filewatcher/linux_filewatcher.cpp

@@ -25,7 +25,7 @@ void LinuxFileWatcher::watchFile() {
     auto lastEventTime =
         std::chrono::steady_clock::now() - std::chrono::seconds(100);
 
-    while (running) {
+    while (running.load(std::memory_order_relaxed)) {
         char buffer[BUF_LEN];
         ssize_t length = read(fd, buffer, BUF_LEN);
         if (length < 0)
@@ -79,7 +79,8 @@ void LinuxFileWatcher::startWatching(const std::string &filepath,
     spdlog::info("Watching dirPath: {} for file path {}", dirPath,
                  path.string());
 
-    wd = inotify_add_watch(fd, dirPath.c_str(), IN_MODIFY);
+    wd = inotify_add_watch(fd, dirPath.c_str(),
+                           IN_CLOSE_WRITE | IN_MOVED_TO);
     if (wd == -1) {
         close(fd);
         throw std::runtime_error("Failed to initialize watch " +
@@ -91,7 +92,7 @@ void LinuxFileWatcher::startWatching(const std::string &filepath,
 
 void LinuxFileWatcher::stopWatching() {
     spdlog ::debug("Stop watching");
-    running = false;
+    running.store(false, std::memory_order_relaxed);
     if (wd != -1)
         inotify_rm_watch(fd, wd);
     // Now it should receive 1 final event which is the

src/filewatcher/mac_filewatcher.cpp

@@ -47,7 +47,7 @@ void MacFileWatcher::fsEventsCallback(
 void MacFileWatcher::startWatching(const std::string &path,
                                    FileChangeCallback cb) {
     this->callback = cb;
-    running = true;
+    running.store(true, std::memory_order_relaxed);
     std::filesystem::path abspath(std::filesystem::absolute(path));
     // So /tmp -> /private/tmp and matchs with the fseventstream paths
     std::filesystem::path canonicalPath = std::filesystem::canonical(abspath);
@@ -76,7 +76,9 @@ void MacFileWatcher::startWatching(const std::string &path,
         FSEventStreamStart(stream);
 
         std::unique_lock<std::mutex> lock(mtx);
-        cv.wait(lock, [this] { return !running; });
+        cv.wait(lock, [this] {
+            return !running.load(std::memory_order_relaxed);
+        });
 
         FSEventStreamStop(stream);
         FSEventStreamInvalidate(stream);
@@ -89,7 +91,7 @@ void MacFileWatcher::startWatching(const std::string &path,
 }
 
 void MacFileWatcher::stopWatching() {
-    running = false;
+    running.store(false, std::memory_order_relaxed);
     cv.notify_one(); // Signal the condition variable to unblock the thread
 
     if (watchThread.joinable()) {

src/filewatcher/windows_filewatcher.cpp

@@ -39,7 +39,7 @@ void WindowsFileWatcher::watchFile() {
     auto lastEventTime = std::chrono::steady_clock::now() - INITIAL_TIME_OFFSET;
 
     // Main loop runs until stopWatching flips the running flag.
-    while (running) {
+    while (running.load(std::memory_order_relaxed)) {
         // Kick off async directory monitoring for writes and renames.
         BOOL success = ReadDirectoryChangesW(
             hDirectory, buffer, BUFFER_SIZE, FALSE,
@@ -184,16 +184,16 @@ void WindowsFileWatcher::startWatching(const std::string &filepath,
                                  std::to_string(GetLastError()));
     }
 
-    running = true;
+    running.store(true, std::memory_order_relaxed);
     watcherThread = std::thread{&WindowsFileWatcher::watchFile, this};
 }
 
 void WindowsFileWatcher::stopWatching() {
     spdlog::debug("Stop watching (Windows)");
-    if (!running)
+    if (!running.load(std::memory_order_relaxed))
         return;
 
-    running = false;
+    running.store(false, std::memory_order_relaxed);
 
     // Signal the stop event to wake up the watching thread
     if (hStopEvent != INVALID_HANDLE_VALUE) {

tests/filewatcher/test_filewatcher.cpp

@@ -9,8 +9,22 @@
 #include <stdexcept>
 #include <thread>
 
-// How long to wait for the callback to be called
-#define THREAD_WAIT_TIME_MS 50
+// A unit for small waits in test actions.
+constexpr int kThreadWaitTimeMs = 50;
+// Wait long enough to confirm no change occurred.
+constexpr int kNoChangeWaitMs = kThreadWaitTimeMs * 2;
+// Max wait to confirm a change occurred.
+// We don't have to wait this long due to kPollIntervalMs below,
+// So early stoppage on an action almost always make tests faster.
+constexpr int kCallbackMaxWaitMs = kThreadWaitTimeMs * 20;
+// A small delay between repeated file replacements.
+constexpr int kBetweenReplacementsWaitMs = kThreadWaitTimeMs;
+// Wait long enough for safe-save rename callback to occur.
+constexpr int kSafeSaveWaitMs = kThreadWaitTimeMs * 4;
+// Polling avoids a fixed long sleep so tests can finish early when callbacks
+// are fast. eg. we dont need to wait for the full kCallbackMaxWaitMs
+// if action returns true early.
+constexpr int kPollIntervalMs = 5;
 
 // Helper function to simulate file modification
 
@@ -47,8 +61,7 @@ void safeSaveFile(const std::string &path, const std::string &content) {
     ec.clear();
     std::filesystem::rename(tempPath, original, ec);
     if (ec) {
-        throw std::runtime_error("Failed to rename temp file: " +
-                                 ec.message());
+        throw std::runtime_error("Failed to rename temp file: " + ec.message());
     }
 }
 
@@ -71,90 +84,115 @@ class FileWatcherTest : public ::testing::Test {
 };
 
 TEST_F(FileWatcherTest, NoChangeCallbackNotCalled) {
-    bool callbackCalled = false;
-    auto callback = [&callbackCalled]() { callbackCalled = true; };
+    std::atomic<bool> callbackCalled{false};
+    auto callback = [&callbackCalled]() { callbackCalled.store(true); };
     createFile(testFilePath, "New content");
     createFile(differentFilePath, "Different content");
-    std::this_thread::sleep_for(std::chrono::milliseconds(THREAD_WAIT_TIME_MS));
+    std::this_thread::sleep_for(std::chrono::milliseconds(kNoChangeWaitMs));
 
     auto watcher = filewatcher_factory::createFileWatcher();
     watcher->startWatching(testFilePath, callback);
     appendToFile(differentFilePath, "New content");
-    std::this_thread::sleep_for(std::chrono::milliseconds(THREAD_WAIT_TIME_MS));
+    std::this_thread::sleep_for(std::chrono::milliseconds(kNoChangeWaitMs));
     watcher->stopWatching();
 
-    EXPECT_FALSE(callbackCalled);
+    EXPECT_FALSE(callbackCalled.load());
 }
 
 TEST_F(FileWatcherTest, FileModifiedCallbackCalled) {
-    bool callbackCalled = false;
-    auto callback = [&callbackCalled]() { callbackCalled = true; };
+    std::atomic<bool> callbackCalled{false};
+    auto callback = [&callbackCalled]() { callbackCalled.store(true); };
     createFile(testFilePath, "New content");
 
     auto watcher = filewatcher_factory::createFileWatcher();
     watcher->startWatching(testFilePath, callback);
-    std::this_thread::sleep_for(std::chrono::milliseconds(THREAD_WAIT_TIME_MS));
+    std::this_thread::sleep_for(std::chrono::milliseconds(kThreadWaitTimeMs));
     appendToFile(testFilePath, "New content");
-    std::this_thread::sleep_for(std::chrono::milliseconds(THREAD_WAIT_TIME_MS));
+
+    // This one sometimes takes a bit longer to trigger eg. on Mac
+    // so use kCallbackMaxWaitMs.
+    // But usually it just finishes really early
+    for (int waitedMs = 0;
+         waitedMs < kCallbackMaxWaitMs && !callbackCalled.load();
+         waitedMs += kPollIntervalMs) {
+        std::this_thread::sleep_for(std::chrono::milliseconds(kPollIntervalMs));
+    }
     watcher->stopWatching();
 
-    EXPECT_TRUE(callbackCalled);
+    EXPECT_TRUE(callbackCalled.load());
 }
 
 TEST_F(FileWatcherTest, FileDeletedAndReplacedCallbackCalled) {
-    bool callbackCalled = false;
-    auto callback = [&callbackCalled]() { callbackCalled = true; };
+    std::atomic<bool> callbackCalled{false};
+    auto callback = [&callbackCalled]() { callbackCalled.store(true); };
     createFile(testFilePath, "New content");
 
     auto watcher = filewatcher_factory::createFileWatcher();
     watcher->startWatching(testFilePath, callback);
-    std::this_thread::sleep_for(std::chrono::milliseconds(THREAD_WAIT_TIME_MS));
+    std::this_thread::sleep_for(std::chrono::milliseconds(kThreadWaitTimeMs));
     replaceFile(testFilePath, "Replacement content");
-    std::this_thread::sleep_for(std::chrono::milliseconds(THREAD_WAIT_TIME_MS));
+
+    // This one sometimes takes a bit longer to trigger eg. on Mac
+    // so use kCallbackMaxWaitMs.
+    // But usually it just finishes really early
+    for (int waitedMs = 0;
+         waitedMs < kCallbackMaxWaitMs && !callbackCalled.load();
+         waitedMs += kPollIntervalMs) {
+        std::this_thread::sleep_for(std::chrono::milliseconds(kPollIntervalMs));
+    }
     watcher->stopWatching();
 
-    EXPECT_TRUE(callbackCalled);
+    EXPECT_TRUE(callbackCalled.load());
 }
 
 TEST_F(FileWatcherTest, FileReplacedMultipleTimesCallbackCalled) {
-    int callbackCount = 0;
-    auto callback = [&callbackCount]() { callbackCount++; };
+    std::atomic<int> callbackCount{0};
+    auto callback = [&callbackCount]() { callbackCount.fetch_add(1); };
 
     createFile(testFilePath, "New content");
     auto watcher = filewatcher_factory::createFileWatcher();
     watcher->startWatching(testFilePath, callback);
-    std::this_thread::sleep_for(std::chrono::milliseconds(50));
+    std::this_thread::sleep_for(std::chrono::milliseconds(kThreadWaitTimeMs));
     for (int i = 0; i < 10; ++i) {
         replaceFile(testFilePath, "Content " + std::to_string(i));
         std::this_thread::sleep_for(
-            std::chrono::milliseconds(50)); // Wait a bit between replacements
+            std::chrono::milliseconds(kBetweenReplacementsWaitMs));
+    }
+    for (int waitedMs = 0;
+         waitedMs < kCallbackMaxWaitMs && callbackCount.load() < 10;
+         waitedMs += kPollIntervalMs) {
+        std::this_thread::sleep_for(std::chrono::milliseconds(kPollIntervalMs));
     }
     watcher->stopWatching();
 
-    EXPECT_GE(callbackCount, 10); // Ensure callback was called at least once
+    EXPECT_GE(callbackCount.load(),
+              10); // Ensure callback was called at least once
 }
 
-// This can be Windows only for now
+// Windows/Linux only for now.
 // In any case the Shader Compiler will raise if it can't find the file
-// So this doesn't seem to be too important eg. on Mac
-#if defined(_WIN32)
+// So this doesn't seem to be too important eg. on Mac.
+// maybe macOS FSEvents stream doesn’t guarantee a clean “removed” flag for
+// every delete, and deletes are maybe reported as a rename (move to trash) or
+// maybe a metadata change before the remove flag appears??
+#if defined(_WIN32) || defined(__linux__)
 TEST_F(FileWatcherTest, FileDeletedDoesNotTriggerCallback) {
     bool callbackCalled = false;
     auto callback = [&callbackCalled]() { callbackCalled = true; };
     createFile(testFilePath, "Initial content");
 
     auto watcher = filewatcher_factory::createFileWatcher();
     watcher->startWatching(testFilePath, callback);
-    std::this_thread::sleep_for(std::chrono::milliseconds(THREAD_WAIT_TIME_MS));
+    std::this_thread::sleep_for(std::chrono::milliseconds(kThreadWaitTimeMs));
     std::remove(testFilePath.c_str());
-    std::this_thread::sleep_for(
-        std::chrono::milliseconds(THREAD_WAIT_TIME_MS * 2));
+    std::this_thread::sleep_for(std::chrono::milliseconds(kNoChangeWaitMs));
     watcher->stopWatching();
 
     EXPECT_FALSE(callbackCalled);
 }
 
 TEST_F(FileWatcherTest, SafeSaveRenameCallbackSeesFile) {
+    // expects at least one callback and no “file couldn’t open” failures
     std::atomic<int> callbackCount{0};
     std::atomic<int> failedOpenCount{0};
     auto callback = [&]() {
@@ -169,10 +207,14 @@ TEST_F(FileWatcherTest, SafeSaveRenameCallbackSeesFile) {
     createFile(testFilePath, "Initial content");
     auto watcher = filewatcher_factory::createFileWatcher();
     watcher->startWatching(testFilePath, callback);
-    std::this_thread::sleep_for(std::chrono::milliseconds(THREAD_WAIT_TIME_MS));
+    std::this_thread::sleep_for(std::chrono::milliseconds(kThreadWaitTimeMs));
     safeSaveFile(testFilePath, "Updated content");
-    std::this_thread::sleep_for(
-        std::chrono::milliseconds(THREAD_WAIT_TIME_MS * 4));
+    for (int waitedMs = 0;
+         waitedMs < kSafeSaveWaitMs && callbackCount.load() == 0 &&
+         failedOpenCount.load() == 0;
+         waitedMs += kPollIntervalMs) {
+        std::this_thread::sleep_for(std::chrono::milliseconds(kPollIntervalMs));
+    }
     watcher->stopWatching();
 
     EXPECT_EQ(failedOpenCount.load(), 0);