Merge pull request #154 from julianmesa-gitkraken/windows-free-handle-on-access-denied

Windows free handle on access denied

Author: julianmesa-gitkraken

includes/win32/Controller.h

@@ -18,9 +18,6 @@ class Controller {
     ~Controller();
   private:
     std::unique_ptr<Watcher> mWatcher;
-
-    HANDLE openDirectory(const std::wstring &path);
-    HANDLE mDirectoryHandle;
 };
 
 #endif

includes/win32/Watcher.h

@@ -14,7 +14,7 @@
 class Watcher
 {
   public:
-    Watcher(std::shared_ptr<EventQueue> queue, HANDLE dirHandle, const std::wstring &path, bool pathWasNtPrefixed);
+    Watcher(std::shared_ptr<EventQueue> queue, const std::wstring &path, bool pathWasNtPrefixed);
     ~Watcher();
 
     bool isRunning() const { return mRunning; }
@@ -29,18 +29,21 @@ class Watcher
     void setError(const std::string &error);
     void eventCallback(DWORD errorCode);
     void handleEvents();
+    void reopenWathedFolder();
+    HANDLE openDirectory(const std::wstring &path);
 
     void resizeBuffers(std::size_t size);
 
     std::string getUTF8Directory(std::wstring path) ;
 
-    std::wstring Watcher::getWatchedPath();
+    std::wstring Watcher::getWatchedPathFromHandle();
     void Watcher::checkWatchedPath();
 
     std::atomic<bool> mRunning;
     SingleshotSemaphore mHasStartedSemaphore;
     SingleshotSemaphore mIsRunningSemaphore;
     mutable std::mutex mErrorMutex;
+    mutable std::mutex mHandleMutex;
     std::string mError;
     std::wstring mWatchedPath;
 

src/win32/Controller.cpp

@@ -39,55 +39,27 @@ static std::wstring convertMultiByteToWideChar(const std::string &multiByte) {
   return wideString;
 }
 
-HANDLE Controller::openDirectory(const std::wstring &path) {
-  return CreateFileW(
-          path.data(),
-          FILE_LIST_DIRECTORY,
-          FILE_SHARE_READ
-          | FILE_SHARE_WRITE
-          | FILE_SHARE_DELETE,
-          NULL,
-          OPEN_EXISTING,
-          FILE_FLAG_BACKUP_SEMANTICS
-          | FILE_FLAG_OVERLAPPED,
-          NULL
-         );
-}
-
-Controller::Controller(std::shared_ptr<EventQueue> queue, const std::string &path)
-  : mDirectoryHandle(INVALID_HANDLE_VALUE)
-{
+Controller::Controller(std::shared_ptr<EventQueue> queue, const std::string &path) {
   auto widePath = convertMultiByteToWideChar(path);
   const bool isNt = isNtPath(widePath);
   if (!isNt) {
     // We convert to an NT Path to support paths > MAX_PATH
     widePath = prefixWithNtPath(widePath);
   }
-  mDirectoryHandle = openDirectory(widePath);
 
-  if (mDirectoryHandle == INVALID_HANDLE_VALUE) {
-    return;
-  }
-
-  mWatcher.reset(new Watcher(queue, mDirectoryHandle, widePath, isNt));
+  mWatcher.reset(new Watcher(queue, widePath, isNt));
 }
 
 Controller::~Controller() {
   mWatcher.reset();
-  CancelIo(mDirectoryHandle);
-  CloseHandle(mDirectoryHandle);
-  mDirectoryHandle = INVALID_HANDLE_VALUE;
 }
 
 std::string Controller::getError() {
-  if (mDirectoryHandle == INVALID_HANDLE_VALUE) {
-    return "Failed to open directory";
-  }
   return mWatcher->getError();
 }
 
 bool Controller::hasErrored() {
-  return mDirectoryHandle == INVALID_HANDLE_VALUE || !mWatcher->getError().empty();
+  return !mWatcher->getError().empty();
 }
 
 bool Controller::isWatching() {

src/win32/Watcher.cpp

@@ -104,22 +104,31 @@ std::string getUTF8FileName(std::wstring path) {
   return utf8Directory;
 }
 
-Watcher::Watcher(std::shared_ptr<EventQueue> queue, HANDLE dirHandle, const std::wstring &path, bool pathWasNtPrefixed)
+Watcher::Watcher(std::shared_ptr<EventQueue> queue, const std::wstring &path, bool pathWasNtPrefixed)
   : mRunning(false),
-  mDirectoryHandle(dirHandle),
   mQueue(queue),
   mPath(path),
   mPathWasNtPrefixed(pathWasNtPrefixed)
 {
-  ZeroMemory(&mOverlapped, sizeof(OVERLAPPED));
-  mOverlapped.hEvent = this;
-  resizeBuffers(1024 * 1024);
-  mWatchedPath = getWatchedPath();
-  start();
+  mDirectoryHandle = openDirectory(mPath);
+  if (mDirectoryHandle == INVALID_HANDLE_VALUE) {
+    setError("Failed to open directory");
+  } else {
+    ZeroMemory(&mOverlapped, sizeof(OVERLAPPED));
+    mOverlapped.hEvent = this;
+    resizeBuffers(1024 * 1024);
+    mWatchedPath = getWatchedPathFromHandle();
+    start();
+  }
 }
 
 Watcher::~Watcher() {
   stop();
+  std::lock_guard<std::mutex> lock(mHandleMutex);
+  if (mDirectoryHandle != INVALID_HANDLE_VALUE) {
+    CancelIo(mDirectoryHandle);
+    CloseHandle(mDirectoryHandle);
+  }
 }
 
 void Watcher::resizeBuffers(std::size_t size) {
@@ -140,6 +149,7 @@ bool Watcher::pollDirectoryChanges() {
     return false;
   }
 
+  std::lock_guard<std::mutex> lock(mHandleMutex);
   if (!ReadDirectoryChangesW(
     mDirectoryHandle,
     mWriteBuffer.data(),
@@ -167,6 +177,39 @@ bool Watcher::pollDirectoryChanges() {
   return true;
 }
 
+HANDLE Watcher::openDirectory(const std::wstring &path) {
+  return CreateFileW(
+          path.data(),
+          FILE_LIST_DIRECTORY,
+          FILE_SHARE_READ
+          | FILE_SHARE_WRITE
+          | FILE_SHARE_DELETE,
+          NULL,
+          OPEN_EXISTING,
+          FILE_FLAG_BACKUP_SEMANTICS
+          | FILE_FLAG_OVERLAPPED,
+          NULL
+         );
+}
+
+void Watcher::reopenWathedFolder() {
+  std::lock_guard<std::mutex> lock(mHandleMutex);
+  {
+    CancelIo(mDirectoryHandle);
+    CloseHandle(mDirectoryHandle);
+    mDirectoryHandle = openDirectory(mPath);
+    if (mDirectoryHandle == INVALID_HANDLE_VALUE) {
+      if(GetFileAttributesW(mWatchedPath.data()) == INVALID_FILE_ATTRIBUTES && GetLastError() == ERROR_PATH_NOT_FOUND) {
+        setError("Service shutdown: root path changed (renamed or deleted)");
+      } else {
+        setError("Service shutdown unexpectedly");
+      }
+      return;
+    }
+  }
+  pollDirectoryChanges();
+}
+
 void Watcher::eventCallback(DWORD errorCode) {
   if (errorCode != ERROR_SUCCESS) {
     if (errorCode == ERROR_NOTIFY_ENUM_DIR) {
@@ -179,7 +222,10 @@ void Watcher::eventCallback(DWORD errorCode) {
         setError("failed resizing buffers for network traffic");
       }
     } else if (errorCode == ERROR_ACCESS_DENIED) {
-      pollDirectoryChanges();
+      // Access denied can happen when we try to delete the watched folder
+      // In Windows Server 2019 and older the folder is not removed until folder handler is released
+      // So the code will reopen the watched folder to release the handler and it will try to open it again
+      reopenWathedFolder();
     } else {
       setError("Service shutdown unexpectedly");
     }
@@ -268,11 +314,13 @@ void Watcher::start() {
 }
 
 void Watcher::stop() {
-  mRunning = false;
-  // schedule a NOOP APC to force the running loop in `Watcher::run()` to wake
-  // up, notice the changed `mRunning` and properly terminate the running loop
-  QueueUserAPC([](__in ULONG_PTR) {}, mRunner.native_handle(), (ULONG_PTR)this);
-  mRunner.join();
+  if (isRunning()) {
+    mRunning = false;
+    // schedule a NOOP APC to force the running loop in `Watcher::run()` to wake
+    // up, notice the changed `mRunning` and properly terminate the running loop
+    QueueUserAPC([](__in ULONG_PTR) {}, mRunner.native_handle(), (ULONG_PTR)this);
+    mRunner.join();
+  }
 }
 
 void Watcher::setError(const std::string &error) {
@@ -298,15 +346,18 @@ std::string Watcher::getError() {
   return mError;
 }
 
-std::wstring Watcher::getWatchedPath() {
+std::wstring Watcher::getWatchedPathFromHandle() {
+  std::lock_guard<std::mutex> lock(mHandleMutex);
+  if (mDirectoryHandle == INVALID_HANDLE_VALUE) {
+    return NULL;
+  }
   DWORD pathLen = GetFinalPathNameByHandleW(mDirectoryHandle, NULL, 0, VOLUME_NAME_NT);
   if (pathLen == 0) {
     setError("Service shutdown: root path changed (renamed or deleted)");
     return NULL;
   }
 
   WCHAR* path = new WCHAR[pathLen];
-
   if (GetFinalPathNameByHandleW(mDirectoryHandle, path, pathLen, VOLUME_NAME_NT) != pathLen - 1) {
     setError("Service shutdown: root path changed (renamed or deleted)");
     delete[] path;
@@ -319,7 +370,7 @@ std::wstring Watcher::getWatchedPath() {
 }
 
 void Watcher::checkWatchedPath() {
-  std::wstring path = getWatchedPath();
+  std::wstring path = getWatchedPathFromHandle();
   if (!path.empty() && path.compare(mWatchedPath) != 0) {
     setError("Service shutdown: root path changed (renamed or deleted)");
   }