2

I've been trying to use and understand the DirectoryWatcher class from Microsoft's Cloud Mirror sample. It uses ReadDirectoryChangesW to monitor changes to a directory. I don't think it's reporting all changes, to be honest. In any event, I had a question about the key part of the code, which is as follows:

concurrency::task<void> DirectoryWatcher::ReadChangesAsync()
{
    auto token = _cancellationTokenSource.get_token();
    return concurrency::create_task([this, token]
    {
        while (true)
        {
            DWORD returned;
            winrt::check_bool(ReadDirectoryChangesW(
                _dir.get(),
                _notify.get(),
                c_bufferSize,
                TRUE,
                FILE_NOTIFY_CHANGE_ATTRIBUTES,
                &returned,
                &_overlapped,
                nullptr));

            DWORD transferred;
            if (GetOverlappedResultEx(_dir.get(), &_overlapped, &transferred, 1000, FALSE))
            {
                std::list<std::wstring> result;
                FILE_NOTIFY_INFORMATION* next = _notify.get();
                while (next != nullptr)
                {
                    std::wstring fullPath(_path);
                    fullPath.append(L"\\");
                    fullPath.append(std::wstring_view(next->FileName, next->FileNameLength / sizeof(wchar_t)));
                    result.push_back(fullPath);

                    if (next->NextEntryOffset)
                    {
                        next = reinterpret_cast<FILE_NOTIFY_INFORMATION*>(reinterpret_cast<char*>(next) + next->NextEntryOffset);
                    }
                    else
                    {
                        next = nullptr;
                    }
                }
                _callback(result);
            }
            else if (GetLastError() != WAIT_TIMEOUT)
            {
                throw winrt::hresult_error(HRESULT_FROM_WIN32(GetLastError()));
            }
            else if (token.is_canceled())
            {
                wprintf(L"watcher cancel received\n");
                concurrency::cancel_current_task();
                return;
            }
        }
    }, token);
}

After reviewing an answer to another question, here's what I don't understand about the code above: isn't the code potentially re-calling ReadDirectoryChangesW before the prior call has returned a result? Or is this code indeed correct? Thanks for any input.

Yes, I seem to have confirmed in my testing that there should be another while loop there around the call to GetOverlappedResultEx, similar to the sample code provided in that other answer. I think the notifications are firing properly with it.

Shouldn't there also be a call to CancelIo in there, too? Or is that not necessary for some reason?

  • yes, code is wrong and `ReadDirectoryChangesW` recalled in case `WAIT_TIMEOUT` when old request still active. and yes - need call `CancelIo` at the end or close file handle (this internally cancel io request). and not need any loops if say true. better use asynchronous callbacks – RbMm Apr 02 '21 at 21:09
  • Asynchronous callbacks add complexity without providing any benefits here. The code is already wrapped inside an asynchronous task, so using a synchronous implementation is perfectly sufficient. – IInspectable Apr 03 '21 at 16:54
  • @IInspectable I had a question about that. If the callbacks are done synchronously, how do you end the thread when it's time to close? –  Apr 03 '21 at 19:29
  • That's what the cancellation token is for that's retrieved in the first line of code in the function. A client can [cancel()](https://learn.microsoft.com/en-us/cpp/parallel/concrt/reference/cancellation-token-source-class#cancel) the cancellation token source. This implementation wakes up once per second to check whether it was cancelled. A fairly crude implementation at that. I'd probably have composed an awaitable object that expires when either a directory change callback arrived, or the cancellation token source was updated. – IInspectable Apr 03 '21 at 19:42
  • It seems like they could've just used a thread-safe boolean in this implementation, right? –  Apr 03 '21 at 19:53
  • what is *complexity* - very relative. and Asynchronous callbacks - this is the most scalable and power solution. it let handle any count of i/o operations with finite number of threads. when this kind of solution require dedicated thread for every file. – RbMm Apr 03 '21 at 20:46
  • @RbMm Do you have an example of the solution you're recommending? –  Apr 03 '21 at 21:12
  • @user15025873 of course. i permanent use this. say [this](https://stackoverflow.com/a/40356818/6401656) or [this](https://stackoverflow.com/a/40600579/6401656) – RbMm Apr 03 '21 at 21:20
  • if want - also https://pastebin.com/dPQpbSJe or https://pastebin.com/HXf7fE28 – RbMm Apr 03 '21 at 21:28

0 Answers0