ReadDirectoryChangesW Improvements

Question

I'm currently using the ReadDirectoryChangesW Function from the Windows API to build a Directory Watcher. The Watcher should monitor a folder for newly added files.

For this i use the following code:

class DirectoryWatcher {
public:
  using Callback = std::function<void(const std::string&)>;
  DirectoryWatcher(const std::filesystem::path& input_directory, std::vector<std::string> file_types, Callback callback);

  explicit operator bool() const {
    return is_valid_;
  }

  bool watch();

  void stop();

  size_t processedFilesCount() const { return processed_files_count; }
  size_t ignoredFilesCount() const { return ignored_files_count; }

private:

  bool eventRecv();
  bool eventSend();
  void handleEvents();
  bool hasEvent() const {
    return event_buf_len_ready_ != 0;
  }
  bool isProcessableFile(const std::filesystem::path& path) const;
  bool isValidAction(int action) const;

  std::filesystem::path input_directory_;
  std::vector<std::string> file_types_{};
  Callback callback_;
  HANDLE path_handle_;
  HANDLE completion_token_{ INVALID_HANDLE_VALUE };
  unsigned long event_buf_len_ready_{ 0 };
  bool is_valid_{ false };
  OVERLAPPED event_overlap_{};
  std::vector<std::byte> event_buf_{ 64 * 1024 };
  size_t processed_files_count{ 0 };
  size_t ignored_files_count{ 0 };

};


DirectoryWatcher::DirectoryWatcher(const std::filesystem::path& input_directory, std::vector<std::string> file_types, Callback callback)
  : input_directory_{ input_directory },
  file_types_{ file_types },
  callback_{ std::move(callback) } {

  path_handle_ = CreateFileA(input_directory.string().c_str(), FILE_LIST_DIRECTORY, FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, nullptr, OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS | FILE_FLAG_OVERLAPPED, nullptr);

  if (path_handle_ != INVALID_HANDLE_VALUE) {
    completion_token_ = CreateIoCompletionPort(INVALID_HANDLE_VALUE, nullptr, 0, 0);
  }

  if (completion_token_ != INVALID_HANDLE_VALUE) {
    is_valid_ = CreateIoCompletionPort(path_handle_, completion_token_, (ULONG_PTR)path_handle_, 1);
  }
}

bool DirectoryWatcher::watch() {
  if (is_valid_) {
    eventRecv();

    while (is_valid_ && hasEvent()) {
      eventSend();
    }

    while (is_valid_) {
      ULONG_PTR completion_key{ 0 };
      LPOVERLAPPED overlap{ nullptr };

      bool complete = GetQueuedCompletionStatus(completion_token_, &event_buf_len_ready_, &completion_key, &overlap, 16);

      if (complete && overlap) {
        handleEvents();
      }
    }
    return true;
  } else {
    return false;
  }


}

void DirectoryWatcher::handleEvents() {
  while (is_valid_ && hasEvent()) {
    eventSend();
    eventRecv();
  }
}

void DirectoryWatcher::stop() {
  is_valid_ = false;
}

bool DirectoryWatcher::eventRecv() {
  event_buf_len_ready_ = 0;
  DWORD bytes_returned = 0;
  memset(&event_overlap_, 0, sizeof(OVERLAPPED));

  auto read_ok = ReadDirectoryChangesW(path_handle_, event_buf_.data(), event_buf_.size(), FALSE, FILE_NOTIFY_CHANGE_FILE_NAME, &bytes_returned, &event_overlap_, nullptr);

  if (!event_buf_.empty() && read_ok) {
    event_buf_len_ready_ = bytes_returned > 0 ? bytes_returned : 0;
    return true;
  }
  if (GetLastError() == ERROR_IO_PENDING) {
    event_buf_len_ready_ = 0;
    is_valid_ = false;
  }
  return false;
}

bool DirectoryWatcher::eventSend() {

  auto buf = reinterpret_cast<FILE_NOTIFY_INFORMATION*>(event_buf_.data());

  if (is_valid_) {
    while (buf + sizeof(FILE_NOTIFY_INFORMATION) <= buf + event_buf_len_ready_) {
      auto filename = input_directory_ / std::wstring{ buf->FileName, buf->FileNameLength / 2 };
      if (isValidAction(buf->Action) && isProcessableFile(filename)) {
        callback_({ filename.string() });
        processed_files_count++;
      } else {
        ignored_files_count++;
      }
      if (buf->NextEntryOffset == 0) {
        break;
      }

      buf = reinterpret_cast<FILE_NOTIFY_INFORMATION*>(reinterpret_cast<std::byte*>(buf) + buf->NextEntryOffset);
    }
    return true;
  } else {
    return false;
  }
}

bool DirectoryWatcher::isProcessableFile(const std::filesystem::path& path) const {
  // Get extension and erase first character (dot-character of extension)
  std::string extension = path.extension().string().erase(0, 1);
  std::transform(extension.begin(), extension.end(), extension.begin(),
    [](unsigned char c) { return std::tolower(c); });
  return std::find(file_types_.begin(), file_types_.end(), extension) != file_types_.end();
}

bool DirectoryWatcher::isValidAction(int action) const {
  return action == FILE_ACTION_ADDED || action == FILE_ACTION_RENAMED_NEW_NAME;
}

int main() {
  int counter{ 0 };
  std::cout << "Hello World!\n";
  DirectoryWatcher watcher{ "C:\\temp\\dummy", {"dat"}, [&counter](const std::string& filename) {
    counter++;
    std::cout << "counter: " << counter << '\n';
    //processing
    std::filesystem::remove(filename);
  } };

  std::thread th1{ &DirectoryWatcher::watch, &watcher };

  std::this_thread::sleep_for(std::chrono::seconds(300));

  watcher.stop();

  th1.join();
}

I think that i don't have the best way used to do such things so i'm happy for any improvements or suggestions on this. The code should be compilable with c++ 17 and Visual Studio.

Answer 1

Simplify the interface

Do you really need all those public member functions? It looks like processedFilesCount() and ignoredFilesCount() is something you might want to use for debugging, but would an application really care about those numbers? If it really cared, you could make the callback update some counters, as you already do in your own example code in main().

If you always intend to run watch() in a separate thread, then you could consider removing watch() and stop() (or make them private), and instead let the constructor automatically start watching and the destructor stop it. Your main() would then look like:

int main() {
    DirectoryWatcher watcher{…};
    std::this_thread::sleep_for(std::chrono::seconds(300));
}

I also wonder if you should pass a list of extensions to watch for to DirectoryWatcher(). The callback could easily check for that itself. And only checking extensions is not very flexible; what if I want to check for files whose name starts with a digit?

Thread safety

When you want to stop a thread, you should use atomic variables or mutexes to coordinate between the thread running watch() and the thread calling stop().

Don't store unnecessary data in member variables

You store lots if information in member variables. However, this then takes up memory even if you aren't actually watching anything. A lot of the member variables are only necessary for watch() itself. They could be made local member variables of that function, and passed down to functions it calls where necessary.

Also, why do you need to store input_directory_? After you have opened path_handle_, your class doesn't actually need the name of the directory anymore; isProcessableFile() only cares about the extension, not the directory name. The user of your class can pass the directory name to the callback function if it is really necessary.

Why is event_buf_ exactly 64 kilobytes?

You hardcoded the size of event_buf_ to 64 kilobytes. But why? FILE_NOTIFY_INFORMATION is just a few integers and the filename, but on Windows, a filename cannot be larger than 255 characters. So the buffer would be hold about 244 FILE_NOTIFY_INFORMATION structs, which seems to be much larger than necessary. How often do you expect that many files to be added between calls to ReadDirectoryChangesW()?

Dodgy pointer arithmetic

In this line of code:

while (buf + sizeof(FILE_NOTIFY_INFORMATION) <= buf + event_buf_len_ready_) {

buf is a pointer to a FILE_NOTIFY_INFORMATION. If you add 1 to buf, it will therefore add sizeof(FILE_NOTIFY_INFORMATION) to the address. You are lucky here because you made the same mistake on both sides of the inequality, which cancels it out. You should instead just write:

while (sizeof(FILE_NOTIFY_INFORMATION) <= event_buf_len_ready_) {

But I also recommend you keep one std::byte* pointer variable that you can update, and then cast that to a FILE_NOTIFY_INFORMATION* only right before you need it. So:

auto ptr = event_buf_.data();
…
while (sizeof(FILE_NOTIFY_INFORMATION) <= event_buf_len_ready_) {
    auto buf = reinterpret_cast<FILE_NOTIFY_INFORMATION*>(ptr);
    …
    ptr += buf->NextEntryOffset;
}

Possible infinite loop

Near the start of watch() you have this loop:

while (is_valid_ && hasEvent()) {
    eventSend();
}

Since eventSend() never clears event_buf_len_ready_, if it ever enters this loop, it will be an infinite one.

Alternatives to callbacks

Callback functions are certainly one way to do something when a directory even occurs, however they have some drawbacks. Especially if they are run in their own thread and might need to interact with other parts of your code base, you then need to worry about thread safety. There are ways to avoid that.

One rather elegant solution is to use a thread-safe queue. The watch() function just pushes filenames to the queue, another thread can pop from it in a loop.