[common] Fix FileWatcherTest once and for all (#53)

But...

ONCE AND FOR ALL!

A recent change introduced WaitForWatching(), which was supposed to
block until the file watcher is actively monitoring the directory.
However this always returned immediately since the watcher is in
kFailed state if the directory was deleted, which counts as watching
(IsStarted returns true for both kWatching and kFailed states).

This CL adds an IsWatching() helper function that returns true only for
the kWatching state, which means that the directory is actively being
watched.

common/file_watcher_win.cc

@@ -65,7 +65,13 @@ int64_t ToUnixTime(LARGE_INTEGER windows_time) {
 // Background thread to read directory changes.
 class AsyncFileWatcher {
  public:
-  enum class FileWatcherState { kDefault, kFailed, kRunning, kShuttingDown };
+  enum class FileWatcherState {
+    kDefault,      // Not started.
+    kFailed,       // Some error during watching, e.g. directory got deleted.
+                   // Will attempt to recover automatically.
+    kWatching,     // Actively watching directory.
+    kShuttingDown  // Shutdown() was called, winding watcher down.
+  };
 
   using FileAction = FileWatcherWin::FileAction;
   using FileInfo = FileWatcherWin::FileInfo;
@@ -158,12 +164,17 @@ class AsyncFileWatcher {
     return dir_recreate_count_;
   }
 
-  bool IsWatching() const ABSL_LOCKS_EXCLUDED(state_mutex_) {
+  bool IsStarted() const ABSL_LOCKS_EXCLUDED(state_mutex_) {
     absl::MutexLock mutex(&state_mutex_);
     return state_ != FileWatcherState::kDefault &&
            state_ != FileWatcherState::kShuttingDown;
   }
 
+  bool IsWatching() const ABSL_LOCKS_EXCLUDED(state_mutex_) {
+    absl::MutexLock mutex(&state_mutex_);
+    return state_ == FileWatcherState::kWatching;
+  }
+
   bool IsShuttingDown() const ABSL_LOCKS_EXCLUDED(state_mutex_) {
     absl::MutexLock mutex(&state_mutex_);
     return state_ == FileWatcherState::kShuttingDown;
@@ -322,7 +333,7 @@ class AsyncFileWatcher {
       return;
     }
 
-    MaybeSetState(FileWatcherState::kRunning);
+    MaybeSetState(FileWatcherState::kWatching);
     // Initialize handles to watch: changes in |dir_path_| and shutdown
     // events.
     HANDLE watch_handles[] = {overlapped.hEvent, shutdown_event_.Get()};
@@ -585,7 +596,7 @@ absl::Status FileWatcherWin::StartWatching(FilesChangedCb files_changed_cb,
   async_watcher_ = std::make_unique<AsyncFileWatcher>(
       dir_path_, std::move(files_changed_cb), std::move(dir_recreated_cb),
       timeout_ms, enforceLegacyReadDirectoryChangesForTesting_);
-  while (GetStatus().ok() && !IsWatching()) {
+  while (GetStatus().ok() && !IsStarted()) {
     std::this_thread::sleep_for(std::chrono::milliseconds(1));
   }
   return GetStatus();
@@ -610,6 +621,10 @@ absl::Status FileWatcherWin::StopWatching() {
   return async_status;
 }
 
+bool FileWatcherWin::IsStarted() const {
+  return async_watcher_ ? async_watcher_->IsStarted() : false;
+}
+
 bool FileWatcherWin::IsWatching() const {
   return async_watcher_ ? async_watcher_->IsWatching() : false;
 }
@@ -627,7 +642,7 @@ uint32_t FileWatcherWin::GetDirRecreateEventCountForTesting() const {
 }
 
 void FileWatcherWin::EnforceLegacyReadDirectoryChangesForTesting() {
-  assert(!IsWatching());
+  assert(!IsStarted());
   enforceLegacyReadDirectoryChangesForTesting_ = true;
 }
 

common/file_watcher_win.h

@@ -77,7 +77,12 @@ class FileWatcherWin {
   // Stops watching directory changes.
   absl::Status StopWatching() ABSL_LOCKS_EXCLUDED(modified_files_mutex_);
 
-  // Indicates whether a directory is currently watched.
+  // Indicates whether StartWatching() was called, but StopWatching() was not
+  // called yet.
+  bool IsStarted() const;
+
+  // Indicates whether a directory is actively watched for changes. In contrast
+  // to IsStarted(), returns false while the directory does not exist.
   bool IsWatching() const;
 
   // Returns the watching status.

common/file_watcher_win_test.cc

@@ -135,8 +135,8 @@ class FileWatcherParameterizedTest : public ::testing::TestWithParam<bool> {
     return changed;
   }
 
-  // Polls for a second until the watcher is watching again.
-  bool WaitForWatching() const {
+  // Polls for a second until the watcher is running again.
+  bool WaitForRunning() const {
     for (int n = 0; n < 1000; ++n) {
       if (watcher_.IsWatching()) return true;
       Util::Sleep(1);
@@ -210,7 +210,7 @@ TEST_P(FileWatcherParameterizedTest, DirDoesNotExist) {
   if (legacyReadDirectoryChanges_)
     watcher_.EnforceLegacyReadDirectoryChangesForTesting();
   EXPECT_NOT_OK(watcher.StartWatching([this]() { OnFilesChanged(); }));
-  EXPECT_FALSE(watcher.IsWatching());
+  EXPECT_FALSE(watcher.IsStarted());
   absl::Status status = watcher.GetStatus();
   EXPECT_NOT_OK(status);
   EXPECT_TRUE(absl::IsFailedPrecondition(status));
@@ -549,8 +549,8 @@ TEST_P(FileWatcherParameterizedTest, RecreateWatchedDir) {
   EXPECT_TRUE(watcher_.GetModifiedFiles().empty());
   EXPECT_OK(watcher_.GetStatus());
 
-  // Wait until the watcher is watching again, or else we might miss the file.
-  EXPECT_TRUE(WaitForWatching());
+  // Wait until the watcher is running again, or else we might miss the file.
+  EXPECT_TRUE(WaitForRunning());
 
   // Creation of a new file should be detected.
   EXPECT_OK(path::WriteFile(first_file_path_, kFirstData, kFirstDataSize));
@@ -584,8 +584,8 @@ TEST_P(FileWatcherParameterizedTest, RecreateUpperDir) {
   EXPECT_TRUE(watcher_.GetModifiedFiles().empty());
   EXPECT_OK(watcher_.GetStatus());
 
-  // Wait until the watcher is watching again, or else we might miss the file.
-  EXPECT_TRUE(WaitForWatching());
+  // Wait until the watcher is running again, or else we might miss the file.
+  EXPECT_TRUE(WaitForRunning());
 
   // Creation of a new file should be detected.
   EXPECT_OK(path::WriteFile(first_file_path_, kFirstData, kFirstDataSize));