Watcher Does not close properly

[KevDi]

Hello,

first of all i want to thank you for the code you provide here this helps me alot because i was looking for some nice example on how to use the ReadDirectoryChangesW Function. I also liked the Post on Medium.

I think i found a little bug in your implementation which could lead to an endless run of the Watcher.

For this i attached the following Function to the changeEvent:

watch.changeEvent = [](int64_t id, const std::set<std::pair<std::wstring, uint32_t>>& notifications) {
    for (const auto& notif : notifications) {
      std::wcout << L"Change on watcher with ID=" << id <<
        L", relative path: \"" << notif.first.c_str() << L"\"" << L"event: " <<
        std::to_wstring(notif.second).c_str() << std::endl;
      std::this_thread::sleep_for(std::chrono::seconds(1));
    }
  };

I just added the Sleep to simulate some long running work based on the File Event.
If you now for example add 10 Files to the Directory and Hit a Key inside the Console Window during the processing of the 10 Files the application tries to close the Eventloop. If you now adding more Files for example to the Directory these events will also processed (which should not happen because the application should close).
You can now make changes to the directory as long as some events are still processed and the Loop will not be closed successfully.

To close the Application successfully i made a change to the FsWatcher::stopEventLoop.
I just switched the Statements and first put the completition Message on the Queue and then try to remove the WatchInfos

This is the version of the Function:

void FsWatcher::stopEventLoop() {
 
  if (this->iocp.get() != INVALID_HANDLE_VALUE) {
    std::cout << "Post Completion Status\n";
    auto res = PostQueuedCompletionStatus(this->iocp.get(), 0, reinterpret_cast<ULONG_PTR>(this), nullptr);
    if (res) {
      std::cout << "Successfully posted completition status\n";
    } else {
      std::cout << "Could not post completition status " << GetLastError() << " \n";
    }
  }

  {
    std::cout << "Stop Event Loop tries to get Lock\n";
    std::lock_guard<std::mutex> lock(this->watchInfoMutex);
    std::cout << "Stop Event Loop gets the lock\n";
    for (auto& watchInfo : this->watchInfos) {
      watchInfo.second.stop();
    }
  }
}

I don't know if this leads to some other problems but with this change the termination of the Program works as expected.

[kovbal]

Hi!

Thanks for reading my blog post, I'm glad you liked it! 😃

If you have event handlers that run for a longer period, then I would advise you to implement some way to make them interruptible. This how we handle these situations in our production codebase. The event loop tries to process all events, even if it should exit, as it waits for the "close" notifications.

I did not manage to reproduce the issue that you mentioned about the application exiting with an error. Can you share more information about it? What was the exact error code, and what was the function that reported it?

I think with your proposed solution, we could not detect when all watches were cancelled, and we would destroy them while they are still listening. As long as they are in Listening state, the OS can write into the buffer, so it's the best to wait for them to receive that "close" notification.

[KevDi]

I can provide you the code tomorrow 👍 But I'm currently interested in how you make the WatchInfo Class interrupt able because in the application I develop it should run for a long time 👍

[KevDi]

About the mentioned problem. The application does not crashes or ends with an error. You just can keep it running for ever after you tried to stopped the application.

This is my main method:

int main(int argc, char* argv[]) {

  FsWatcher watch;
  watch.changeEvent = [](int64_t id, const std::set<std::pair<std::string, std::string>>& notifications) {
    for (const auto& notif : notifications) {
      std::cout << "Change on watcher with ID=" << id <<
        ", path: \"" << notif.first.c_str() << "\\" << notif.second.c_str() <<"\"" << std::endl;
      std::this_thread::sleep_for(std::chrono::seconds(4));
    }
  };
  watch.errorEvent = [](int64_t id) {
    std::cout << "An error has occurred, no further events will be sent for ID= " << id << std::endl;
  };

  std::cout << "Listening for changes on the following directories:\n";
  watch.addDirectory(1, "D:\\SomeFolder");

The importent part is the std::this_thread::sleep_for call in the Callback.
After you started the application do the following:

• Add multiple Files (4-5) to the Watched Folder.
• Don't wait for the application to finish handling all the Files
• Press a key to go beyond the std::getchar
• The application should now try to clean up and close itself.
• Keep adding files to the Directory

Every file you added now to the Directory will still be handled and the application will never close until you stop adding Files.
This is because of the stopEventLoop. In this method it first tries to lock the Mutex what is not possible because the mutex is already locked by the WatchInfo because of the Event Handling. At this point the application waits until all events are processed but if you keep adding Files to the Directory this will never happen.
If you first put the Close Message on the Queue and then call the stop Method on the WatchInfo the application closes as expected.

Hope it's clear to unterstand the problem 😂

[kovbal]

Thanks for your detailed explanation!

I'll try to create a pull request in the coming days that stops the event loop even if the event handler runs for longer.