From 5f75c363c46e6cc35ca8cdaa072f4b16d1b603fc Mon Sep 17 00:00:00 2001 From: David Morgan Date: Tue, 18 Nov 2025 13:10:00 +0100 Subject: [PATCH] Improve e2e test, fix Linux watcher for moves between directories. Fix MacOS watcher for recently-created directories. --- pkgs/watcher/CHANGELOG.md | 25 ++++-- .../src/directory_watcher/directory_list.dart | 85 +++++++++++++++++-- .../lib/src/directory_watcher/linux.dart | 50 +++++++++-- .../lib/src/directory_watcher/mac_os.dart | 13 ++- .../directory_watcher/end_to_end_tests.dart | 12 +-- .../test/directory_watcher/file_changer.dart | 31 +++---- .../test/directory_watcher/file_tests.dart | 25 ++++++ pkgs/watcher/test/utils.dart | 16 +++- 8 files changed, 201 insertions(+), 56 deletions(-) diff --git a/pkgs/watcher/CHANGELOG.md b/pkgs/watcher/CHANGELOG.md index 4ae096190..d4ae7a863 100644 --- a/pkgs/watcher/CHANGELOG.md +++ b/pkgs/watcher/CHANGELOG.md @@ -10,15 +10,16 @@ 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: fix tracking failure on Linux. Before the fix, renaming a directory - would cause subdirectories of that directory to no longer be tracked. -- Bug fix: 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. -- 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. +- Bug fix: fix `DirectoryWatcher` tracking failure on Linux. Before the fix, + renaming a directory would cause subdirectories of that directory to no + longer be tracked. +- Bug fix: fix `DirectoryWatcher` incorrect events on Linux when a file or + directory is moved between directories then immediately modified or deleted. +- Bug fix: fix `DirectoryWatcher` duplicate ADD events on Linux when a file + is created in a recently moved or created directory. +- 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. - 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. @@ -30,8 +31,14 @@ - Bug fix: with `DirectoryWatcher` on Windows, new links to direcories were sometimes incorrectly handled as actual directories. Now they are reported as files, matching the behavior of the Linux and MacOS watchers. +- 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 diff --git a/pkgs/watcher/lib/src/directory_watcher/directory_list.dart b/pkgs/watcher/lib/src/directory_watcher/directory_list.dart index 7d84201f5..a54f2a156 100644 --- a/pkgs/watcher/lib/src/directory_watcher/directory_list.dart +++ b/pkgs/watcher/lib/src/directory_watcher/directory_list.dart @@ -142,16 +142,12 @@ class _ResolvedDirectory { } on FileSystemException catch (e, s) { // The first operation on a directory is to resolve symbolic links, which // fails with a general FileSystemException if the file is not found. - // Convert that into a PathNotFoundException or PathAccessException - // as that makes more sense to the caller, who didn't ask for anything to - // do with symbolic links. + // See https://github.com/dart-lang/sdk/issues/61946. Use a copy of the + // SDK codepath that should convert it to a more specific type. if (e.message.contains('Cannot resolve symbolic links')) { - if (e.osError?.errorCode == 2) { - throw Error.throwWithStackTrace( - PathNotFoundException(directory.path, e.osError!), s); - } else if (e.osError?.errorCode == 5) { - throw Error.throwWithStackTrace( - PathAccessException(directory.path, e.osError!), s); + if (e.osError != null && e.path != null) { + Error.throwWithStackTrace( + _fromOSError(e.osError!, e.message, e.path!), s); } } rethrow; @@ -160,3 +156,74 @@ class _ResolvedDirectory { bool get isCanonical => canonicalPath == directory.path; } + +// Copied from sdk/lib/io/file.dart. +FileSystemException _fromOSError(OSError err, String message, String path) { + if (Platform.isWindows) { + switch (err.errorCode) { + case _errorAccessDenied: + case _errorCurrentDirectory: + case _errorWriteProtect: + case _errorBadLength: + case _errorSharingViolation: + case _errorLockViolation: + case _errorNetworkAccessDenied: + case _errorDriveLocked: + return PathAccessException(path, err, message); + case _errorFileExists: + case _errorAlreadyExists: + return PathExistsException(path, err, message); + case _errorFileNotFound: + case _errorPathNotFound: + case _errorInvalidDrive: + case _errorInvalidName: + case _errorNoMoreFiles: + case _errorBadNetpath: + case _errorBadNetName: + case _errorBadPathName: + case _errorFilenameExedRange: + return PathNotFoundException(path, err, message); + default: + return FileSystemException(message, path, err); + } + } else { + switch (err.errorCode) { + case _ePerm: + case _eAccess: + return PathAccessException(path, err, message); + case _eExist: + return PathExistsException(path, err, message); + case _eNoEnt: + return PathNotFoundException(path, err, message); + default: + return FileSystemException(message, path, err); + } + } +} + +// Copied from sdk/lib/io/common.dart. POSIX. +const _ePerm = 1; +const _eNoEnt = 2; +const _eAccess = 13; +const _eExist = 17; + +// Copied from sdk/lib/io/common.dart. Windows. +const _errorFileNotFound = 2; +const _errorPathNotFound = 3; +const _errorAccessDenied = 5; +const _errorInvalidDrive = 15; +const _errorCurrentDirectory = 16; +const _errorNoMoreFiles = 18; +const _errorWriteProtect = 19; +const _errorBadLength = 24; +const _errorSharingViolation = 32; +const _errorLockViolation = 33; +const _errorBadNetpath = 53; +const _errorNetworkAccessDenied = 65; +const _errorBadNetName = 67; +const _errorFileExists = 80; +const _errorDriveLocked = 108; +const _errorInvalidName = 123; +const _errorBadPathName = 161; +const _errorAlreadyExists = 183; +const _errorFilenameExedRange = 206; diff --git a/pkgs/watcher/lib/src/directory_watcher/linux.dart b/pkgs/watcher/lib/src/directory_watcher/linux.dart index ad2cccd2a..daec02b4c 100644 --- a/pkgs/watcher/lib/src/directory_watcher/linux.dart +++ b/pkgs/watcher/lib/src/directory_watcher/linux.dart @@ -154,10 +154,14 @@ class _LinuxDirectoryWatcher var dirs = {}; var changed = {}; - // inotify event batches are ordered by occurrence, so we treat them as a - // log of what happened to a file. We only emit events based on the - // difference between the state before the batch and the state after it, not - // the intermediate state. + // Inotify events are usually ordered by occurrence. But, + // https://github.com/dart-lang/sdk/issues/62014 means that moves between + // directories cause create/delete events to be placed out of order at the + // end of the batch. Catch these cases in order to do a check on the actual + // filesystem state. + var deletes = {}; + var creates = {}; + for (var event in batch) { // If the watched directory is deleted or moved, we'll get a deletion // event for it. Ignore it; we handle closing [this] when the underlying @@ -168,41 +172,71 @@ class _LinuxDirectoryWatcher switch (event.type) { case EventType.moveFile: + deletes.add(event.path); files.remove(event.path); dirs.remove(event.path); var destination = event.destination; if (destination != null) { + creates.add(destination); changed.add(destination); files.add(destination); dirs.remove(destination); } case EventType.moveDirectory: + deletes.add(event.path); files.remove(event.path); dirs.remove(event.path); var destination = event.destination; if (destination != null) { + creates.add(destination); changed.add(destination); files.remove(destination); dirs.add(destination); } case EventType.delete: + deletes.add(event.path); files.remove(event.path); dirs.remove(event.path); case EventType.createDirectory: + creates.add(event.path); + files.remove(event.path); + dirs.add(event.path); + case EventType.modifyDirectory: files.remove(event.path); dirs.add(event.path); case EventType.createFile: + creates.add(event.path); + files.add(event.path); + dirs.remove(event.path); + case EventType.modifyFile: files.add(event.path); dirs.remove(event.path); } } + // Check paths that might have been affected by out-of-order events, set + // the correct state in [files] and [dirs]. + for (final path in deletes.intersection(creates)) { + final type = FileSystemEntity.typeSync(path, followLinks: false); + if (type == FileSystemEntityType.file || + type == FileSystemEntityType.link) { + files.add(path); + dirs.remove(path); + } else if (type == FileSystemEntityType.directory) { + dirs.add(path); + files.remove(path); + } else { + files.remove(path); + dirs.remove(path); + } + } + _applyChanges(files, dirs, changed); } @@ -244,8 +278,12 @@ class _LinuxDirectoryWatcher if (entity is Directory) { _watchSubdir(entity.path); } else { - _files.add(entity.path); - _emitEvent(ChangeType.ADD, entity.path); + // Only emit ADD if it hasn't already been emitted due to the file being + // modified or added after the directory was added. + if (!_files.contains(entity.path)) { + _files.add(entity.path); + _emitEvent(ChangeType.ADD, entity.path); + } } }, onError: (Object error, StackTrace stackTrace) { // Ignore an exception caused by the dir not existing. It's fine if it diff --git a/pkgs/watcher/lib/src/directory_watcher/mac_os.dart b/pkgs/watcher/lib/src/directory_watcher/mac_os.dart index e9a26ebcf..495dacb01 100644 --- a/pkgs/watcher/lib/src/directory_watcher/mac_os.dart +++ b/pkgs/watcher/lib/src/directory_watcher/mac_os.dart @@ -136,10 +136,10 @@ class _MacOSDirectoryWatcher for (var event in events) { switch (event.type) { case EventType.createFile: - // If we already know about the file, treat it like a modification. - // This can happen if a file is copied on top of an existing one. - // We'll see an ADD event for the latter file when from the user's - // perspective, the file's contents just changed. + case EventType.modifyFile: + // The type can be incorrect due to a race with listing a new + // directory or due to a file being copied over an existing one. + // Choose the type to emit based on the previous emitted state. var type = _files.contains(path) ? ChangeType.MODIFY : ChangeType.ADD; @@ -152,7 +152,7 @@ class _MacOSDirectoryWatcher var stream = Directory(path).listRecursivelyIgnoringErrors(); var subscription = stream.listen((entity) { if (entity is Directory) return; - if (_files.contains(path)) return; + if (_files.contains(entity.path)) return; _emitEvent(ChangeType.ADD, entity.path); _files.add(entity.path); @@ -163,9 +163,6 @@ class _MacOSDirectoryWatcher subscription.onError(_emitError); _listSubscriptions.add(subscription); - case EventType.modifyFile: - _emitEvent(ChangeType.MODIFY, path); - case EventType.delete: for (var removedPath in _files.remove(path)) { _emitEvent(ChangeType.REMOVE, removedPath); diff --git a/pkgs/watcher/test/directory_watcher/end_to_end_tests.dart b/pkgs/watcher/test/directory_watcher/end_to_end_tests.dart index bf62f6fcd..99adec4c4 100644 --- a/pkgs/watcher/test/directory_watcher/end_to_end_tests.dart +++ b/pkgs/watcher/test/directory_watcher/end_to_end_tests.dart @@ -27,10 +27,6 @@ void endToEndTests({required bool isNative}) { final temp = Directory.systemTemp.createTempSync(); addTearDown(() => temp.deleteSync(recursive: true)); - // Start with some files. - final changer = FileChanger(temp.path); - await changer.changeFiles(times: 100); - // Create the watcher and [ClientSimulator]. final watcher = createWatcher(path: temp.path); final client = await ClientSimulator.watch(watcher); @@ -38,9 +34,13 @@ void endToEndTests({required bool isNative}) { // 20 iterations of making changes, waiting for events to settle, and // checking for consistency. - for (var i = 0; i != 20; ++i) { + final changer = FileChanger(temp.path); + for (var i = 0; i != 40; ++i) { + for (final entity in temp.listSync()) { + entity.deleteSync(recursive: true); + } // File changes. - final messages = await changer.changeFiles(times: 100); + final messages = await changer.changeFiles(times: 200); // Give time for events to arrive. To allow tests to run quickly when the // events are handled quickly, poll and continue if verification passes. diff --git a/pkgs/watcher/test/directory_watcher/file_changer.dart b/pkgs/watcher/test/directory_watcher/file_changer.dart index 775d89a65..00a9bb1da 100644 --- a/pkgs/watcher/test/directory_watcher/file_changer.dart +++ b/pkgs/watcher/test/directory_watcher/file_changer.dart @@ -8,6 +8,8 @@ import 'dart:math'; import 'package:path/path.dart' as p; +import '../utils.dart'; + /// Changes files randomly. /// /// Writes are done in an isolate so as to not block the watcher code being @@ -23,7 +25,7 @@ import 'package:path/path.dart' as p; class FileChanger { final String path; - final Random _random = Random(0); + Random _random = Random(0); final List _messages = []; FileChanger(this.path); @@ -31,8 +33,13 @@ class FileChanger { /// Changes files under [path], [times] times. /// /// Returns a log of the changes made. - Future> changeFiles({required int times}) async => - await Isolate.run(() => _changeFiles(times: times)); + Future> changeFiles({required int times}) async { + final result = await Isolate.run(() => _changeFiles(times: times)); + // The `Random` instance gets copied to the isolate on every run, so by + // default it will produce the same numbers. Update it to get new numbers. + _random = Random(_random.nextInt(0xffffffff)); + return result; + } Future> _changeFiles({required int times}) async { _messages.clear(); @@ -82,7 +89,7 @@ class FileChanger { final existingPath2 = _randomExistingFilePath()!; _log('move file over file,$existingPath,$existingPath2'); // Fails sometimes on Windows, so guard+retry. - _retryForPathAccessException( + retryForPathAccessException( () => File(existingPath).renameSync(existingPath2)); case 6: @@ -94,7 +101,7 @@ class FileChanger { _ensureParent(newDirectory); _log('move directory to new,$existingDirectory,$newDirectory'); // Fails sometimes on Windows, so guard+retry. - _retryForPathAccessException( + retryForPathAccessException( () => Directory(existingDirectory).renameSync(newDirectory)); case 7: @@ -133,6 +140,7 @@ class FileChanger { /// Returns the path to an already-created file, or `null` if none exists. String? _randomExistingFilePath() => (Directory(path).listSync(recursive: true).whereType().toList() + ..sort((a, b) => a.path.compareTo(b.path)) ..shuffle(_random)) .firstOrNull ?.path; @@ -142,6 +150,7 @@ class FileChanger { String? _randomExistingDirectoryPath() => (Directory( path, ).listSync(recursive: true).whereType().toList() + ..sort((a, b) => a.path.compareTo(b.path)) ..shuffle(_random)) .firstOrNull ?.path; @@ -156,16 +165,4 @@ class FileChanger { message = message.replaceAll(',$path${Platform.pathSeparator}', ','); _messages.add(message); } - - /// Retries [action] until it does not throw [PathAccessException]. - void _retryForPathAccessException(void Function() action) { - while (true) { - try { - action(); - return; - } on PathAccessException catch (e) { - print('Temporary failure, retrying: $e'); - } - } - } } diff --git a/pkgs/watcher/test/directory_watcher/file_tests.dart b/pkgs/watcher/test/directory_watcher/file_tests.dart index 0fe5e8008..827ac147b 100644 --- a/pkgs/watcher/test/directory_watcher/file_tests.dart +++ b/pkgs/watcher/test/directory_watcher/file_tests.dart @@ -251,6 +251,31 @@ void _fileTests({required bool isNative}) { deleteFile('file.txt'); }); + test('reports when a file is moved between directories then deleted', + () async { + writeFile('a/test.txt'); + createDir('b'); + await startWatcher(path: 'b'); + + renameFile('a/test.txt', 'b/test.txt'); + deleteFile('b/test.txt'); + + final events = + await takeEvents(duration: const Duration(milliseconds: 500)); + + // It's correct to report either nothing or an add+remove. + expect( + events, + anyOf([ + isEmpty, + containsAll([ + isAddEvent('b/test.txt'), + isRemoveEvent('b/test.txt'), + ]), + ])); + expect(events, isNot(contains(isModifyEvent('b/test.txt')))); + }); + test( 'reports a modification when a file is deleted and then immediately ' 'recreated', () async { diff --git a/pkgs/watcher/test/utils.dart b/pkgs/watcher/test/utils.dart index 8d0a999c5..2cd0b9155 100644 --- a/pkgs/watcher/test/utils.dart +++ b/pkgs/watcher/test/utils.dart @@ -306,7 +306,9 @@ void createDir(String path) { /// Renames a directory in the sandbox from [from] to [to]. void renameDir(String from, String to) { var absoluteTo = p.join(d.sandbox, to); - Directory(p.join(d.sandbox, from)).renameSync(absoluteTo); + // Fails sometimes on Windows, so guard+retry. + retryForPathAccessException( + () => Directory(p.join(d.sandbox, from)).renameSync(absoluteTo)); expect(FileSystemEntity.typeSync(absoluteTo, followLinks: false), FileSystemEntityType.directory); } @@ -337,3 +339,15 @@ Set withPermutations(S Function(int, int, int) callback, {int? limit}) { } return results; } + +/// Retries [action] until it does not throw [PathAccessException]. +void retryForPathAccessException(void Function() action) { + while (true) { + try { + action(); + return; + } on PathAccessException catch (e) { + print('Temporary failure, retrying: $e'); + } + } +}