Windows filewatcher: switch to ReadDirectoryChangesW.

Based on danimo's #2454 fix for #2455 and related to #2297.

src/mirall/folderwatcher_win.cpp

@@ -13,6 +13,7 @@
 
 #include <QThread>
 #include <QDebug>
+#include <QDir>
 
 #include "mirall/folderwatcher.h"
 #include "mirall/folderwatcher_win.h"
@@ -23,52 +24,123 @@
 
 namespace Mirall {
 
-void WatcherThread::run()
+void WatcherThread::watchChanges(size_t fileNotifyBufferSize,
+                                 bool* increaseBufferSize)
 {
-    _handle = FindFirstChangeNotification((wchar_t*)_path.utf16(),
+    *increaseBufferSize = false;
-                                         true, // recursive watch
+
-                                         FILE_NOTIFY_CHANGE_FILE_NAME |
+    _handle = CreateFileW(
-                                         FILE_NOTIFY_CHANGE_DIR_NAME |
+        (wchar_t*)_path.utf16(),
-                                         FILE_NOTIFY_CHANGE_LAST_WRITE);
+        FILE_LIST_DIRECTORY,
+        FILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE,
+        NULL,
+        OPEN_EXISTING,
+        FILE_FLAG_BACKUP_SEMANTICS,
+        NULL
+        );
 
     if (_handle == INVALID_HANDLE_VALUE)
     {
-        qDebug() << Q_FUNC_INFO << "FindFirstChangeNotification function failed, stopping watcher!";
+        DWORD errorCode = GetLastError();
-        FindCloseChangeNotification(_handle);
+        qDebug() << Q_FUNC_INFO << "Failed to create handle for" << _path << ", error:" << errorCode;
         _handle = 0;
         return;
     }
 
-    if (_handle == NULL)
+    // QVarLengthArray ensures the stack-buffer is aligned like double and qint64.
-    {
+    QVarLengthArray<char, 4096*10> fileNotifyBuffer;
-        qDebug() << Q_FUNC_INFO << "FindFirstChangeNotification returned null, stopping watcher!";
+    fileNotifyBuffer.resize(fileNotifyBufferSize);
-        FindCloseChangeNotification(_handle);
-        _handle = 0;
-        return;
-    }
 
-    while(true) {
+    const size_t fileNameBufferSize = 4096;
-        switch(WaitForSingleObject(_handle, /*wait*/ INFINITE)) {
+    TCHAR fileNameBuffer[fileNameBufferSize];
-        case WAIT_OBJECT_0:
+
-            if (FindNextChangeNotification(_handle) == false) {
+    forever {
-                qDebug() << Q_FUNC_INFO << "FindFirstChangeNotification returned FALSE, stopping watcher!";
+        FILE_NOTIFY_INFORMATION *pFileNotifyBuffer =
-                FindCloseChangeNotification(_handle);
+                (FILE_NOTIFY_INFORMATION*)fileNotifyBuffer.data();
-                _handle = 0;
+        DWORD dwBytesReturned = 0;
-                return;
+        SecureZeroMemory(pFileNotifyBuffer, fileNotifyBufferSize);
+        if(ReadDirectoryChangesW( _handle, (LPVOID)pFileNotifyBuffer,
+                                  fileNotifyBufferSize, true,
+                                  FILE_NOTIFY_CHANGE_FILE_NAME |
+                                  FILE_NOTIFY_CHANGE_DIR_NAME |
+                                  FILE_NOTIFY_CHANGE_LAST_WRITE,
+                                  &dwBytesReturned, NULL, NULL))
+        {
+            FILE_NOTIFY_INFORMATION *curEntry = pFileNotifyBuffer;
+            forever {
+                size_t len = curEntry->FileNameLength / 2;
+                QString file = _path + "\\" + QString::fromWCharArray(curEntry->FileName, len);
+
+                // Unless the file was removed or renamed, get its full long name
+                // TODO: We could still try expanding the path in the tricky cases...
+                QString longfile = file;
+                if (curEntry->Action != FILE_ACTION_REMOVED
+                        && curEntry->Action != FILE_ACTION_RENAMED_OLD_NAME) {
+                    size_t longNameSize = GetLongPathNameW(reinterpret_cast<LPCWSTR>(file.utf16()), fileNameBuffer, fileNameBufferSize);
+                    if (longNameSize > 0) {
+                        longfile = QString::fromUtf16(reinterpret_cast<const ushort *>(fileNameBuffer), longNameSize);
+                    } else {
+                        qDebug() << Q_FUNC_INFO << "Error converting file name to full length, keeping original name.";
+                    }
+                }
+                longfile = QDir::cleanPath(longfile);
+
+                qDebug() << Q_FUNC_INFO << "Found change in" << longfile << "action:" << curEntry->Action;
+                emit changed(longfile);
+
+                if (curEntry->NextEntryOffset == 0) {
+                    break;
+                }
+                curEntry = (FILE_NOTIFY_INFORMATION*)(
+                                (char*)curEntry + curEntry->NextEntryOffset);
             }
-            // qDebug() << Q_FUNC_INFO << "Change detected in" << _path << "from" << QThread::currentThread    ();
+        } else {
-            emit changed(_path);
+            DWORD errorCode = GetLastError();
-            break;
+            switch(errorCode) {
-        default:
+            case ERROR_NOTIFY_ENUM_DIR:
-            qDebug()  << Q_FUNC_INFO << "Error while watching";
+                qDebug() << Q_FUNC_INFO << "The buffer for changes overflowed! Triggering a generic change and resizing";
+                emit changed(_path);
+                *increaseBufferSize = true;
+                break;
+            default:
+                qDebug() << Q_FUNC_INFO << "General error" << errorCode << "while watching. Exiting.";
+                break;
+            }
+            CloseHandle(_handle);
+            _handle = NULL;
+            return;
+        }
+    }
+}
+
+void WatcherThread::run()
+{
+    // If this buffer fills up before we've extracted its data we will lose
+    // change information. Therefore start big.
+    size_t bufferSize = 4096*10;
+    size_t maxBuffer = 64*1024;
+
+    forever {
+        bool increaseBufferSize = false;
+        watchChanges(bufferSize, &increaseBufferSize);
+
+        if (increaseBufferSize) {
+            bufferSize = qMin(bufferSize*2, maxBuffer);
+        } else {
+            // Other errors shouldn't actually happen,
+            // so sleep a bit to avoid running into the same error case in a
+            // tight loop.
+            sleep(2);
         }
     }
 }
 
 WatcherThread::~WatcherThread()
 {
-    if (_handle)
+    if (_handle) {
-        FindCloseChangeNotification(_handle);
+        CloseHandle(_handle);
+        _handle = NULL;
+    }
 }
 
 FolderWatcherPrivate::FolderWatcherPrivate(FolderWatcher *p, const QString& path)

src/mirall/folderwatcher_win.h

@@ -33,6 +33,8 @@ public:
 
 protected:
     void run();
+    void watchChanges(size_t fileNotifyBufferSize,
+                      bool* increaseBufferSize);
 
 signals:
     void changed(const QString &path);

test/testfolderwatcher.h

@@ -15,15 +15,23 @@
 
 using namespace Mirall;
 
+class FriendlyThread : public QThread
+{
+    friend class TestFolderWatcher;
+};
+
 class TestFolderWatcher : public QObject
 {
     Q_OBJECT
 
 public slots:
     void slotFolderChanged( const QString& path ) {
-        qDebug() << "COMPARE: " << path << _checkMark;
+        if (_skipNotifications.contains(path)) {
-        QVERIFY(_checkMark == path);
+            return;
-        _checkMark.clear();
+        }
+        if (_requiredNotifications.contains(path)) {
+            _receivedNotifications.insert(path);
+        }
     }
 
     void slotEnd() { // in case something goes wrong...
@@ -36,7 +44,16 @@ private:
     FolderWatcher *_watcher;
     QEventLoop     _loop;
     QTimer         _timer;
-    QString        _checkMark;
+    QSet<QString>  _requiredNotifications;
+    QSet<QString>  _receivedNotifications;
+    QSet<QString>  _skipNotifications;
+
+    void processAndWait()
+    {
+        _loop.processEvents();
+        FriendlyThread::msleep(200);
+        _loop.processEvents();
+    }
 
 private slots:
     void initTestCase() {
@@ -52,68 +69,95 @@ private slots:
         rootDir.mkpath(_root + "/a2/b3/c3");
         Utility::writeRandomFile( _root+"/a1/random.bin");
         Utility::writeRandomFile( _root+"/a1/b2/todelete.bin");
-        Utility::writeRandomFile( _root+"/a2/movefile");
+        Utility::writeRandomFile( _root+"/a2/renamefile");
+        Utility::writeRandomFile( _root+"/a1/movefile");
 
         _watcher = new FolderWatcher(_root);
         QObject::connect(_watcher, SIGNAL(folderChanged(QString)), this, SLOT(slotFolderChanged(QString)));
-        _timer.singleShot(3000, this, SLOT(slotEnd()));
+        _timer.singleShot(5000, this, SLOT(slotEnd()));
+    }
+
+    void init()
+    {
+        _receivedNotifications.clear();
+        _requiredNotifications.clear();
+        _skipNotifications.clear();
+    }
+
+    void checkNotifications()
+    {
+        processAndWait();
+        QCOMPARE(_receivedNotifications, _requiredNotifications);
     }
 
     void testACreate() { // create a new file
         QString cmd;
-        _checkMark = _root;
+        _requiredNotifications.insert(_root);
         cmd = QString("echo \"xyz\" > %1/foo.txt").arg(_root);
         qDebug() << "Command: " << cmd;
         system(cmd.toLocal8Bit());
 
-        _loop.processEvents();
+        checkNotifications();
-        QVERIFY(_checkMark.isEmpty()); // the slot clears the checkmark.
     }
 
     void testATouch() { // touch an existing file.
+        _requiredNotifications.insert(_root+"/a1");
+#ifdef Q_OS_WIN
+        Utility::writeRandomFile(QString("%1/a1/random.bin").arg(_root));
+#else
         QString cmd;
         cmd = QString("/usr/bin/touch %1/a1/random.bin").arg(_root);
-        _checkMark = _root+"/a1";
         qDebug() << "Command: " << cmd;
         system(cmd.toLocal8Bit());
+#endif
 
-        _loop.processEvents();
+        checkNotifications();
-        QVERIFY(_checkMark.isEmpty()); // the slot clears the checkmark.
     }
 
     void testCreateADir() {
-        _checkMark = _root+"/a1/b1";
+        _requiredNotifications.insert(_root+"/a1/b1");
+        _skipNotifications.insert(_root + "/a1/b1/new_dir");
         QDir dir;
         dir.mkdir( _root + "/a1/b1/new_dir");
         QVERIFY(QFile::exists(_root + "/a1/b1/new_dir"));
-        _loop.processEvents();
+
-        QVERIFY(_checkMark.isEmpty()); // the slot clears the checkmark.
+        checkNotifications();
     }
 
     void testRemoveADir() {
-        _checkMark = _root+"/a1/b3";
+        _requiredNotifications.insert(_root+"/a1/b3");
         QDir dir;
         QVERIFY(dir.rmdir(_root+"/a1/b3/c3"));
-        _loop.processEvents();
+
-        QVERIFY(_checkMark.isEmpty()); // the slot clears the checkmark.
+        checkNotifications();
     }
 
     void testRemoveAFile() {
-        _checkMark = _root+"/a1/b2";
+        _requiredNotifications.insert(_root+"/a1/b2");
         QVERIFY(QFile::exists(_root+"/a1/b2/todelete.bin"));
         QFile::remove(_root+"/a1/b2/todelete.bin");
         QVERIFY(!QFile::exists(_root+"/a1/b2/todelete.bin"));
-        _loop.processEvents();
+
-        QVERIFY(_checkMark.isEmpty()); // the slot clears the checkmark.
+        checkNotifications();
+    }
+
+    void testRenameAFile() {
+        _requiredNotifications.insert(_root+"/a2");
+        QVERIFY(QFile::exists(_root+"/a2/renamefile"));
+        QFile::rename(_root+"/a2/renamefile", _root+"/a2/renamefile.renamed" );
+        QVERIFY(QFile::exists(_root+"/a2/renamefile.renamed"));
+
+        checkNotifications();
     }
 
     void testMoveAFile() {
-        _checkMark = _root+"/a2";
+        _requiredNotifications.insert(_root+"/a1");
-        QVERIFY(QFile::exists(_root+"/a2/movefile"));
+        _requiredNotifications.insert(_root+"/a2");
-        QFile::rename(_root+"/a2/movefile", _root+"/a2/movefile.renamed" );
+        QVERIFY(QFile::exists(_root+"/a1/movefile"));
+        QFile::rename(_root+"/a1/movefile", _root+"/a2/movefile.renamed" );
         QVERIFY(QFile::exists(_root+"/a2/movefile.renamed"));
-        _loop.processEvents();
+
-        QVERIFY(_checkMark.isEmpty()); // the slot clears the checkmark.
+        checkNotifications();
     }
 
     void cleanupTestCase() {