0

I am calling ReadDirectoryChangesW asynchronously to monitor directory changes in a background thread.

This how the directory (basePath) is opened and the "reading" thread is started:

    m_hDIR = CreateFileW(
            basePath,
            FILE_LIST_DIRECTORY | GENERIC_READ,
            FILE_SHARE_WRITE | FILE_SHARE_READ,
            NULL,
            OPEN_EXISTING,
            FILE_FLAG_BACKUP_SEMANTICS | FILE_FLAG_OVERLAPPED,
            NULL);

        if (m_hDIR == INVALID_HANDLE_VALUE)
            throw CrException(CrWin32ErrorString());

        //Start reading changes in background thread
        m_Callback = std::move(a_Callback);
        m_Reading = true;
        m_ReadThread = std::thread(&CrDirectoryWatcher::StartRead, this);

This is StartRead(): (Note: m_Reading is atomic<bool>)

void StartRead()
        {
            DWORD dwBytes = 0;
            FILE_NOTIFY_INFORMATION fni{0};
            OVERLAPPED o{0};

            //Be sure to set the hEvent member of the OVERLAPPED structure to a unique event.
            o.hEvent = CreateEvent(0, 0, 0, 0);

            while(m_Reading)
            {
                if (!ReadDirectoryChangesW(m_hDIR,
                    &fni, sizeof(fni),
                    TRUE, FILE_NOTIFY_CHANGE_LAST_WRITE,
                    &dwBytes, &o, NULL))
                {
                    CrAssert(0, CrWin32ErrorString());
                }

                if (!GetOverlappedResult(m_hDIR, &o, &dwBytes, FALSE))
                    CrAssert(0, CrWin32ErrorString());

                if (fni.Action != 0)
                {
                    std::wstring fileName(fni.FileName, fni.FileNameLength);
                    m_Callback(fileName);
                    fni.Action = 0;
                }
            }
        }

Basically, I am "polling" for new changes every frame. Now when I call GetOverlappedResult() it fails and yields the following error:

Overlapped I/O event is not in a signaled state.

Am I missing something? Is ReadDirectoryChangesW meant to be called every "tick"? Or just when new changes were detected?

Note: When I leave out the OVERLAPPED struct (and GetOverlappedResult) it works, but blocks the thread until changes were read. This prevents my application to properly terminate. (i.e. I can't join the thread)

  • Have you tried `if (!GetOverlappedResult(m_hDIR, &o, &dwBytes, TRUE))` - if I'm reading the [`GetOverlappedResult` function doc](https://msdn.microsoft.com/en-us/library/windows/desktop/ms683209(v=vs.85).aspx) correctly, that should block your thread until there's actually something to read. – Phil Brubaker Apr 27 '17 at 18:13
  • You cannot get the result until the operation completes. That allows you to do something else, but the basic problem is that you are not doing anything else. The code insist on getting the result right away. So no real point in using overlapped I/O. Look at RegisterWaitForSingleObject() to (potentially) make this a bit more like you intended. Or use a worker thread. – Hans Passant Apr 27 '17 at 18:13
  • 1
    really you call ReadDirectoryChangesW synchronously. use event in OVERLAPPED the worst way - compare IOCP and apc – RbMm Apr 27 '17 at 18:15
  • @PhilBrubaker But that would result in the thread being "unjoinable" right? –  Apr 27 '17 at 18:16
  • @HansPassant Does RegisterWaitForSingleObject() work with OVERLAPPED? because without providing the struct ReadDirectoryChangesW blocks. –  Apr 27 '17 at 18:23
  • @mutex36 If the issue is blocking forever, then yes that's a concern. You need a way to cancel the overlapped I/O and exit your thread function nicely. – Phil Brubaker Apr 27 '17 at 18:27
  • `RegisterWaitForSingleObject` work with event - it will be signaled and your *Callback* called when operation finished. but this not give you pointer to overlapped back - you will be have no knowledge which operation finished (if you use more than one async io). in case *IOCP* or *APC* you will pointer to your overlapped in callback – RbMm Apr 27 '17 at 18:28

1 Answers1

8

When calling GetOverlappedResult(), if you set the bWait parameter to FALSE and the I/O operation hasn't completed yet, GetOverlappedResult() fails with an ERROR_IO_INCOMPLETE error code:

bWait [in]
If this parameter is TRUE, and the Internal member of the lpOverlapped structure is STATUS_PENDING, the function does not return until the operation has been completed. If this parameter is FALSE and the operation is still pending, the function returns FALSE and the GetLastError function returns ERROR_IO_INCOMPLETE.

That is not a fatal error, so just ignore that error and move on.

And yes, make sure you don't call ReadDirectoryChangesW() again until GetOverlappedResult() has reported the previous I/O operation has completed first.

Now, with that said, there is another problem with your code. Your thread is allocating a single FILE_NOTIFY_INFORMATION instance on the stack. If you look at the definition of FILE_NOTIFY_INFORMATION, its FileName field is variable-length:

typedef struct _FILE_NOTIFY_INFORMATION {
  DWORD NextEntryOffset;
  DWORD Action;
  DWORD FileNameLength;
  WCHAR FileName[1];
} FILE_NOTIFY_INFORMATION, *PFILE_NOTIFY_INFORMATION;

FileName
A variable-length field that contains the file name relative to the directory handle. The file name is in the Unicode character format and is not null-terminated.

Which means allocating a FILE_NOTIFY_INFORMATION statically is going to be too small, and dwBytes will almost always be 0 since ReadDirectoryChangesW() won't be able to return a full FILE_NOTIFY_INFORMATION to you (unless the FileName is exactly 1 character in length):

When you first call ReadDirectoryChangesW, the system allocates a buffer to store change information. This buffer is associated with the directory handle until it is closed and its size does not change during its lifetime. Directory changes that occur between calls to this function are added to the buffer and then returned with the next call. If the buffer overflows, the entire contents of the buffer are discarded, the lpBytesReturned parameter contains zero, and the ReadDirectoryChangesW function fails with the error code ERROR_NOTIFY_ENUM_DIR.

ERROR_NOTIFY_ENUM_DIR
1022 (0x3FE)
A notify change request is being completed and the information is not being returned in the caller's buffer. The caller now needs to enumerate the files to find the changes.

So, you need to dynamically allocate a large byte buffer for receiving FILE_NOTIFY_INFORMATION data, and then you can walk that buffer whenever GetOverlappedResult() reports that data is available.

Your thread should look something more like this:

void StartRead()
{
    DWORD dwBytes = 0;
    std::vector<BYTE> buffer(1024*64);
    OVERLAPPED o{0};
    bool bPending = false;

    //Be sure to set the hEvent member of the OVERLAPPED structure to a unique event.
    o.hEvent = CreateEvent(NULL, TRUE, FALSE, NULL);
    if (!o.hEvent) {
        CrAssert(0, CrWin32ErrorString());
    }

    while (m_Reading)
    {
        bPending = ReadDirectoryChangesW(m_hDIR,
            &buffer[0], buffer.size(),
            TRUE, FILE_NOTIFY_CHANGE_LAST_WRITE,
            &dwBytes, &o, NULL);
        if (!bPending)
        {
            CrAssert(0, CrWin32ErrorString());
        }

        while (m_Reading)
        {
            if (GetOverlappedResult(m_hDIR, &o, &dwBytes, FALSE))
            {
                bPending = false;

                if (dwBytes != 0)
                {
                    FILE_NOTIFY_INFORMATION *fni = reinterpret_cast<FILE_NOTIFY_INFORMATION*>(&buffer[0]);
                    do
                    {
                        if (fni->Action != 0)
                        {
                            std::wstring fileName(fni->FileName, fni->FileNameLength);
                            m_Callback(fileName);
                        }

                        if (fni->NextEntryOffset == 0)
                            break;

                        fni = reinterpret_cast<FILE_NOTIFY_INFORMATION*>(reinterpret_cast<BYTE*>(fni) + fni->NextEntryOffset);
                    }
                    while (true);
                }

                break;
            }

            if (GetLastError() != ERROR_IO_INCOMPLETE) {
                CrAssert(0, CrWin32ErrorString());
            }

            Sleep(10);
        }

        if (bPending)
        {
            CancelIo(m_hDIR);
            GetOverlappedResult(m_hDIR, &o, &dwBytes, TRUE);
        }
    }

    CloseHandle(o.hEvent);
}

An alternative way to implement this without polling the I/O status regularly would be to get rid of m_Reading and use a waitable event instead. Let the OS signal the thread when it should call GetOverlappedResult() or terminate, that way it can sleep the rest of the time it is not busy doing something:

m_hDIR = CreateFileW(
            basePath,
            FILE_LIST_DIRECTORY | GENERIC_READ,
            FILE_SHARE_WRITE | FILE_SHARE_READ,
            NULL,
            OPEN_EXISTING,
            FILE_FLAG_BACKUP_SEMANTICS | FILE_FLAG_OVERLAPPED,
            NULL);

if (m_hDIR == INVALID_HANDLE_VALUE)
    throw CrException(CrWin32ErrorString());

m_TermEvent = CreateEvent(NULL, TRUE, FALSE, NULL);
if (!m_TermEvent)
    throw CrException(CrWin32ErrorString());

//Start reading changes in background thread
m_Callback = std::move(a_Callback);
m_ReadThread = std::thread(&CrDirectoryWatcher::StartRead, this);

...

SetEvent(m_TermEvent);
m_ReadThread.join();

void StartRead()
{
    DWORD dwBytes = 0;
    std::vector<BYTE> buffer(1024*64);
    OVERLAPPED o{0};
    bool bPending = false, bKeepRunning = true;

    //Be sure to set the hEvent member of the OVERLAPPED structure to a unique event.
    o.hEvent = CreateEvent(NULL, TRUE, FALSE, NULL);
    if (!o.hEvent) {
        CrAssert(0, CrWin32ErrorString());
    }

    HANDLE h[2] = {o.hEvent, h_TermEvent};

    do
    {
        bPending = ReadDirectoryChangesW(m_hDIR,
            &buffer[0], buffer.size(),
            TRUE, FILE_NOTIFY_CHANGE_LAST_WRITE,
            &dwBytes, &o, NULL);
        if (!bPending)
        {
            CrAssert(0, CrWin32ErrorString());
        }

        switch (WaitForMultipleObjects(2, h, FALSE, INFINITE))
        {
            case WAIT_OBJECT_0:
            {
                if (!GetOverlappedResult(m_hDIR, &o, &dwBytes, TRUE)) {
                    CrAssert(0, CrWin32ErrorString());
                }

                bPending = false;

                if (dwBytes == 0)
                    break;

                FILE_NOTIFY_INFORMATION *fni = reinterpret_cast<FILE_NOTIFY_INFORMATION*>(&buffer[0]);
                do
                {
                    if (fni->Action != 0)
                    {
                        std::wstring fileName(fni->FileName, fni->FileNameLength);
                        m_Callback(fileName);
                    }

                    if (fni->NextEntryOffset == 0)
                         break;

                    fni = reinterpret_cast<FILE_NOTIFY_INFORMATION*>(reinterpret_cast<BYTE*>(fni) + fni->NextEntryOffset);
                }
                while (true);

                break;
            }

            case WAIT_OBJECT_0+1:
                bKeepRunning = false;
                break;

            case WAIT_FAILED:
                CrAssert(0, CrWin32ErrorString());
                break;
        }
    }
    while (bKeepRunning);

    if (bPending)
    {
        CancelIo(m_hDIR);
        GetOverlappedResult(m_hDIR, &o, &dwBytes, TRUE);
    }

    CloseHandle(o.hEvent);
}
Remy Lebeau
  • 555,201
  • 31
  • 458
  • 770
  • Why closing hEvent? –  Apr 27 '17 at 18:27
  • 2
    @mutex36: so it is not leaked. Open kernel handles are closed automatically during process termination, but who says the thread has to be stopped only during app shutdown? The thread creates the event object, it should release the object before exiting, regardless of how the thread is being used. – Remy Lebeau Apr 27 '17 at 18:34
  • Out of curiosity: What happens if you dont GetOverlappedResult() the remaining/pending data? –  Apr 27 '17 at 19:29
  • 1
    @mutex36: it will sit in the I/O queue for the open directory object until retrieved or until the queue/directory is closed. – Remy Lebeau Apr 27 '17 at 19:35
  • so technically it's ok to just CloseHandle() my Directory? –  Apr 27 '17 at 20:02
  • 3
    @mutex36 you should always read the result of an async operation, even if it is canceled. Since it is async, you don't know if it is still running, and should not rip away anything that it is still referring to. – Remy Lebeau Apr 27 '17 at 20:35
  • Let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/142862/discussion-between-mutex36-and-remy-lebeau). –  Apr 27 '17 at 23:00
  • There's a bug in your do-while loop -- you decide whether to break out based on the information *after* the pointer is advanced. This will exit the loop one iteration too early. – Ben Voigt Jul 23 '17 at 00:13
  • @BenVoigt fixed – Remy Lebeau Jul 23 '17 at 16:23