Make link handling consistent across native watchers. (#2261)

Author: davidmorgan

pkgs/watcher/CHANGELOG.md

@@ -10,13 +10,18 @@
   exhaustion, "Directory watcher closed unexpectedly", much less likely. The old
   implementation which does not use a separate Isolate is available as
   `DirectoryWatcher(path, runInIsolateOnWindows: false)`.
-- Bug fix: new `DirectoryWatcher` implementation on Linux that fixes various
-  issues: tracking failure following subdirectory move, incorrect events when
-  there are changes in a recently-moved subdirectory, incorrect events due to
-  various situations involving subdirectory moves.
-- Bug fix: in `DirectoryWatcher` while listing directories skip symlinks that
-  lead to a directory that has already been listed. This prevents a severe
-  performance regression on MacOS and Linux when there are more than a few symlink loops.
+- Document behavior on Linux if the system watcher limit is hit.
+- Bug fix: native `DirectoryWatcher` implementations now consistently handle
+  links as files, instead of sometimes reading through them and sometimes
+  reporting them as files. The polling `DirectoryWatcher` still reads through
+  links.
+- Bug fix: with the polling `DirectoryWatcher`, fix spurious modify event
+  emitted because of a file delete during polling.
+- Bug fix: due to the link handling change, native `DirectoryWatcher` on Linux
+  and MacOS is no longer affected by a severe performance regression if there
+  are symlink loops in the watched directory. The polling `DirectoryWatcher`
+  is fixed to skip already-visited directories to prevent the performance issue
+  while still reading through links.
 - Bug fix: with `DirectoryWatcher` on Windows, the last of a rapid sequence of
   modifications in a newly-created directory was sometimes dropped. Make it
   reliably report the last modification.
@@ -25,18 +30,19 @@
   moved onto `b`, it would be reported as three events: delete `a`, delete `b`,
   create `b`. Now it's reported as two events: delete `a`, modify `b`. This
   matches the behavior of the Linux and MacOS watchers.
-- Bug fix: with `DirectoryWatcher` on Windows, new links to direcories were
+- Bug fix: with `DirectoryWatcher` on Windows, new links to directories were
   sometimes incorrectly handled as actual directories. Now they are reported
   as files, matching the behavior of the Linux and MacOS watchers.
+- Bug fix: new `DirectoryWatcher` implementation on Linux that fixes various
+  issues: tracking failure following subdirectory move, incorrect events when
+  there are changes in a recently-moved subdirectory, incorrect events due to
+  various situations involving subdirectory moves.
 - Bug fix: with `DirectoryWatcher` on MacOS, fix events for changes in new
   directories: don't emit duplicate ADD, don't emit MODIFY without ADD.
-- Bug fix: with `PollingDirectoryWatcher`, fix spurious modify event emitted
-  because of a file delete during polling.
 - Bug fix: with `FileWatcher` on MacOS, a modify event was sometimes reported if
   the file was created immediately before the watcher was created. Now, if the
   file exists when the watcher is created then this modify event is not sent.
   This matches the Linux native and polling (Windows) watchers.
-- Document behavior on Linux if the system watcher limit is hit.
 
 ## 1.1.4
 

pkgs/watcher/lib/src/directory_watcher/directory_list.dart

@@ -15,16 +15,19 @@ extension DirectoryRobustRecursiveListing on Directory {
   /// These can arise from concurrent file-system modification.
   ///
   /// See [listRecursively] for how symlinks are handled.
-  Stream<FileSystemEntity> listRecursivelyIgnoringErrors() {
-    return listRecursively()
+  Stream<FileSystemEntity> listRecursivelyIgnoringErrors(
+      {bool followLinks = true}) {
+    return listRecursively(followLinks: followLinks)
         .ignoring<PathNotFoundException>()
         .ignoring<PathAccessException>();
   }
 
   /// Lists the directory recursively.
   ///
-  /// This is like `Directory.list(recursive: true)`, but handles symlinks like
-  /// `find -L` to avoid a performance issue with symbolic link cycles.
+  /// If you pass `followLinks: false` then this exactly calls
+  /// `Directory.list(recursive: true, followLinks: false)`. If not, it is like
+  ///  `Directory.list(recursive: true)`, but handles symlinks like `find -L` to
+  /// avoid a performance issue with symbolic link cycles.
   ///
   /// See: https://github.com/dart-lang/sdk/issues/61407.
   ///
@@ -33,8 +36,11 @@ extension DirectoryRobustRecursiveListing on Directory {
   /// symlink-resolved paths.
   ///
   /// Skipped links to directories are not mentioned in the directory listing.
-  Stream<FileSystemEntity> listRecursively() =>
-      _DirectoryTraversal(this).listRecursively();
+  Stream<FileSystemEntity> listRecursively({bool followLinks = true}) {
+    return followLinks
+        ? _DirectoryTraversal(this).listRecursively()
+        : list(recursive: true, followLinks: false);
+  }
 }
 
 /// A recursive directory listing algorithm that follows symlinks carefully.

pkgs/watcher/lib/src/directory_watcher/mac_os.dart

@@ -151,7 +151,8 @@ class _MacOSDirectoryWatcher
           case EventType.createDirectory:
             if (_files.containsDir(path)) continue;
 
-            var stream = Directory(path).listRecursivelyIgnoringErrors();
+            var stream = Directory(path)
+                .listRecursivelyIgnoringErrors(followLinks: false);
             var subscription = stream.listen((entity) {
               if (entity is Directory) return;
               if (_files.contains(entity.path)) return;
@@ -336,7 +337,8 @@ class _MacOSDirectoryWatcher
 
     _files.clear();
     var completer = Completer<void>();
-    var stream = Directory(path).listRecursivelyIgnoringErrors();
+    var stream =
+        Directory(path).listRecursivelyIgnoringErrors(followLinks: false);
     _initialListSubscription = stream.listen((entity) {
       if (entity is! Directory) _files.add(entity.path);
     }, onError: _emitError, onDone: completer.complete, cancelOnError: true);

pkgs/watcher/lib/src/directory_watcher/windows.dart

@@ -229,7 +229,8 @@ class WindowsManuallyClosedDirectoryWatcher
           _files.add(path);
 
         case EventType.createDirectory:
-          final stream = Directory(path).listRecursivelyIgnoringErrors();
+          final stream =
+              Directory(path).listRecursivelyIgnoringErrors(followLinks: false);
           final subscription = stream.listen((entity) {
             if (entity is Directory) return;
             if (_files.contains(entity.path)) return;
@@ -375,7 +376,8 @@ class WindowsManuallyClosedDirectoryWatcher
 
     _files.clear();
     var completer = Completer<void>();
-    var stream = Directory(path).listRecursivelyIgnoringErrors();
+    var stream =
+        Directory(path).listRecursivelyIgnoringErrors(followLinks: false);
     void handleEntity(FileSystemEntity entity) {
       if (entity is! Directory) _files.add(entity.path);
     }

pkgs/watcher/test/directory_watcher/end_to_end_tests.dart

@@ -21,7 +21,7 @@ import 'file_changer.dart';
 /// until a failure, it outputs a log which can be turned into a test case here.
 void endToEndTests() {
   // Random test to cover a wide range of cases.
-  test('end to end test: random', timeout: const Timeout(Duration(minutes: 5)),
+  test('end to end test: random', timeout: const Timeout(Duration(minutes: 10)),
       () async {
     await runTest(name: 'random', repeats: 100);
   });
@@ -30,7 +30,7 @@ void endToEndTests() {
   for (final testCase in testCases) {
     test('end to end test: ${testCase.name}',
         timeout: const Timeout(Duration(minutes: 5)), () async {
-      await runTest(name: testCase.name, replayLog: testCase.log, repeats: 100);
+      await runTest(name: testCase.name, replayLog: testCase.log, repeats: 50);
     }, skip: testCase.skipOnLinux && Platform.isLinux);
   }
 }

pkgs/watcher/test/directory_watcher/link_tests.dart

@@ -2,8 +2,6 @@
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
-import 'dart:io';
-
 import 'package:test/test.dart';
 
 import '../utils.dart';
@@ -66,7 +64,7 @@ void _linkTests({required bool isNative}) {
     await startWatcher(path: 'links');
     writeFile('targets/a.target', contents: 'modified');
 
-    // TODO(davidmorgan): reconcile differences.
+    // Native watchers treat links as files, polling watcher polls through them.
     if (isNative) {
       await expectNoEvents();
     } else {
@@ -83,7 +81,7 @@ void _linkTests({required bool isNative}) {
 
     deleteFile('targets/a.target');
 
-    // TODO(davidmorgan): reconcile differences.
+    // Native watchers treat links as files, polling watcher polls through them.
     if (isNative) {
       await expectNoEvents();
     } else {
@@ -115,7 +113,7 @@ void _linkTests({required bool isNative}) {
         target: 'targets/a.targetdir',
         unawaitedAsync: true);
 
-    // TODO(davidmorgan): reconcile differences.
+    // Native watchers treat links as files, polling watcher polls through them.
     if (isNative) {
       await expectAddEvent('links/a.link');
     } else {
@@ -137,7 +135,7 @@ void _linkTests({required bool isNative}) {
         target: 'targets/a.targetdir',
         unawaitedAsync: true);
 
-    // TODO(davidmorgan): reconcile differences.
+    // Native watchers treat links as files, polling watcher polls through them.
     if (isNative) {
       await expectAddEvent('links/a.link');
     } else {
@@ -154,10 +152,28 @@ void _linkTests({required bool isNative}) {
 
     writeFile('targets/a.targetdir/a.txt');
 
-    if (!isNative) {
+    // Native watchers treat links as files, polling watcher polls through them.
+    if (isNative) {
+      await expectNoEvents();
+    } else {
       await expectAddEvent('links/a.link/a.txt');
+    }
+  });
+
+  test('notifies when a file is added to a newly linked directory', () async {
+    createDir('targets');
+    createDir('links');
+    createDir('targets/a.targetdir');
+    await startWatcher(path: 'links');
+
+    writeLink(link: 'links/a.link', target: 'targets/a.targetdir');
+    writeFile('targets/a.targetdir/a.txt');
+
+    // Native watchers treat links as files, polling watcher polls through them.
+    if (isNative) {
+      await expectAddEvent('links/a.link');
     } else {
-      await expectNoEvents();
+      await expectAddEvent('links/a.link/a.txt');
     }
   });
 
@@ -174,8 +190,8 @@ void _linkTests({required bool isNative}) {
 
     renameDir('links', 'watched/links');
 
-    // TODO(davidmorgan): reconcile differences.
-    if (isNative && Platform.isLinux) {
+    // Native watchers treat links as files, polling watcher polls through them.
+    if (isNative) {
       await expectAddEvent('watched/links/a.link');
     } else {
       await expectAddEvent('watched/links/a.link/a.txt');
@@ -197,8 +213,8 @@ void _linkTests({required bool isNative}) {
 
     renameDir('links', 'watched/links');
 
-    // TODO(davidmorgan): reconcile differences.
-    if (isNative && Platform.isLinux) {
+    // Native watchers treat links as files, polling watcher polls through them.
+    if (isNative) {
       await expectAddEvent('watched/links/a.link');
     } else {
       await expectAddEvent('watched/links/a.link/a.txt');
@@ -223,8 +239,8 @@ void _linkTests({required bool isNative}) {
 
     renameDir('links', 'watched/links');
 
-    // TODO(davidmorgan): reconcile diffences.
-    if (isNative && Platform.isLinux) {
+    // Native watchers treat links as files, polling watcher polls through them.
+    if (isNative) {
       await expectAddEvent('watched/links/a.link');
     } else {
       await expectAddEvent('watched/links/a.link/a.txt');