From 86b49ebcb0e61139252e95d67d8b1c6ecadaff06 Mon Sep 17 00:00:00 2001 From: Danny Tuppeny Date: Wed, 22 Nov 2023 17:33:11 +0000 Subject: [PATCH 1/7] Fix FileWatcher.ready for files that don't exist There were a few issues here: - FileWatcher.ready never fired for files that don't exist because of logic inside FileWatcher existing early if the modification time was `null` - The test I recently added trying to catch this was incorrectly passing because the mock timestamp code was set so that files that had not been created would return a 0-mtime whereas in the real implementation they return `null` So this change a) updates the mock to return `null` for uncreated files (to match the real implementation) which breaks a bunch of tests b) fixes those tests by updating FileWatcher._poll() to handle `null` mtime separately from being the first poll. --- CHANGELOG.md | 2 ++ lib/src/file_watcher/polling.dart | 31 +++++++++++++++++++------------ lib/src/stat.dart | 2 +- test/utils.dart | 2 +- 4 files changed, 23 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7743fc7..7c022f4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,7 @@ ## 1.1.1-wip +- Ensure `FileWatcher.ready` completes for files that do not exist. + ## 1.1.0 - Require Dart SDK >= 3.0.0 diff --git a/lib/src/file_watcher/polling.dart b/lib/src/file_watcher/polling.dart index 6f1eee4..5787d53 100644 --- a/lib/src/file_watcher/polling.dart +++ b/lib/src/file_watcher/polling.dart @@ -37,10 +37,15 @@ class _PollingFileWatcher implements FileWatcher, ManuallyClosedWatcher { /// The timer that controls polling. late final Timer _timer; + /// Track whether the next [_poll] is the first. + /// + /// We cannot just check [_lastModified] is null, because that is a valid + /// state for a file that does not exist. + var _isFirstPoll = true; + /// The previous modification time of the file. /// - /// Used to tell when the file was modified. This is `null` before the file's - /// mtime has first been checked. + /// `null` indicates the file does not (or did not on the last poll) exist. DateTime? _lastModified; _PollingFileWatcher(this.path, Duration pollingDelay) { @@ -50,13 +55,13 @@ class _PollingFileWatcher implements FileWatcher, ManuallyClosedWatcher { /// Checks the mtime of the file and whether it's been removed. Future _poll() async { - // We don't mark the file as removed if this is the first poll (indicated by - // [_lastModified] being null). Instead, below we forward the dart:io error - // that comes from trying to read the mtime below. + // We don't mark the file as removed if this is the first poll. Instead, + // below we forward the dart:io error that comes from trying to read the + // mtime below. var pathExists = await File(path).exists(); if (_eventsController.isClosed) return; - if (_lastModified != null && !pathExists) { + if (!_isFirstPoll && !pathExists) { _eventsController.add(WatchEvent(ChangeType.REMOVE, path)); unawaited(close()); return; @@ -73,17 +78,19 @@ class _PollingFileWatcher implements FileWatcher, ManuallyClosedWatcher { } if (_eventsController.isClosed) return; - if (_lastModified == modified) return; - - if (_lastModified == null) { + if (_isFirstPoll) { // If this is the first poll, don't emit an event, just set the last mtime // and complete the completer. _lastModified = modified; _readyCompleter.complete(); - } else { - _lastModified = modified; - _eventsController.add(WatchEvent(ChangeType.MODIFY, path)); + _isFirstPoll = false; + return; } + + if (_lastModified == modified) return; + + _lastModified = modified; + _eventsController.add(WatchEvent(ChangeType.MODIFY, path)); } @override diff --git a/lib/src/stat.dart b/lib/src/stat.dart index 06e3feb..fe0f155 100644 --- a/lib/src/stat.dart +++ b/lib/src/stat.dart @@ -6,7 +6,7 @@ import 'dart:io'; /// A function that takes a file path and returns the last modified time for /// the file at that path. -typedef MockTimeCallback = DateTime Function(String path); +typedef MockTimeCallback = DateTime? Function(String path); MockTimeCallback? _mockTimeCallback; diff --git a/test/utils.dart b/test/utils.dart index 214d669..54363c7 100644 --- a/test/utils.dart +++ b/test/utils.dart @@ -67,7 +67,7 @@ Future startWatcher({String? path}) async { 'Path is not in the sandbox: $path not in ${d.sandbox}'); var mtime = _mockFileModificationTimes[normalized]; - return DateTime.fromMillisecondsSinceEpoch(mtime ?? 0); + return mtime != null ? DateTime.fromMillisecondsSinceEpoch(mtime) : null; }); // We want to wait until we're ready *after* we subscribe to the watcher's From da0ea1fae2f62cb772582916dd74a9e517f917a8 Mon Sep 17 00:00:00 2001 From: Danny Tuppeny Date: Wed, 22 Nov 2023 17:34:07 +0000 Subject: [PATCH 2/7] Update CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7c022f4..7893daf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,6 @@ ## 1.1.1-wip -- Ensure `FileWatcher.ready` completes for files that do not exist. +- Ensure `PollingFileWatcher.ready` completes for files that do not exist. ## 1.1.0 From 3144b90832889c2589a21c4676e1c83da3b45a98 Mon Sep 17 00:00:00 2001 From: Danny Tuppeny Date: Wed, 22 Nov 2023 18:15:05 +0000 Subject: [PATCH 3/7] Fix handling of timestamps for renames/delete+add --- test/utils.dart | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/test/utils.dart b/test/utils.dart index 54363c7..80b993c 100644 --- a/test/utils.dart +++ b/test/utils.dart @@ -195,6 +195,11 @@ Future expectRemoveEvent(String path) => Future allowModifyEvent(String path) => _expectOrCollect(mayEmit(isWatchEvent(ChangeType.MODIFY, path))); +/// Track a fake timestamp to be used when writing files. This always increases +/// so that files that are deleted and re-created do not have their timestamp +/// set back to a previously used value. +int _nextTimestamp = 1; + /// Schedules writing a file in the sandbox at [path] with [contents]. /// /// If [contents] is omitted, creates an empty file. If [updateModified] is @@ -216,14 +221,15 @@ void writeFile(String path, {String? contents, bool? updateModified}) { if (updateModified) { path = p.normalize(path); - _mockFileModificationTimes.update(path, (value) => value + 1, - ifAbsent: () => 1); + _mockFileModificationTimes[path] = _nextTimestamp++; } } /// Schedules deleting a file in the sandbox at [path]. void deleteFile(String path) { File(p.join(d.sandbox, path)).deleteSync(); + + _mockFileModificationTimes.remove(path); } /// Schedules renaming a file in the sandbox from [from] to [to]. @@ -245,6 +251,17 @@ void createDir(String path) { /// Schedules renaming a directory in the sandbox from [from] to [to]. void renameDir(String from, String to) { Directory(p.join(d.sandbox, from)).renameSync(p.join(d.sandbox, to)); + + // Migrate timestamps for any files in this folder. + final knownFilePaths = _mockFileModificationTimes.keys.toList(); + for (final filePath in knownFilePaths) { + if (p.isWithin(from, filePath)) { + print('moving $filePath to ${filePath.replaceAll(from, to)}'); + _mockFileModificationTimes[filePath.replaceAll(from, to)] = + _mockFileModificationTimes[filePath]!; + _mockFileModificationTimes.remove(filePath); + } + } } /// Schedules deleting a directory in the sandbox at [path]. From 3c1372071cf65afc6c2b098797a90e0793c650ff Mon Sep 17 00:00:00 2001 From: Danny Tuppeny Date: Wed, 22 Nov 2023 18:18:42 +0000 Subject: [PATCH 4/7] Remove debug print --- test/utils.dart | 1 - 1 file changed, 1 deletion(-) diff --git a/test/utils.dart b/test/utils.dart index 80b993c..7867b9f 100644 --- a/test/utils.dart +++ b/test/utils.dart @@ -256,7 +256,6 @@ void renameDir(String from, String to) { final knownFilePaths = _mockFileModificationTimes.keys.toList(); for (final filePath in knownFilePaths) { if (p.isWithin(from, filePath)) { - print('moving $filePath to ${filePath.replaceAll(from, to)}'); _mockFileModificationTimes[filePath.replaceAll(from, to)] = _mockFileModificationTimes[filePath]!; _mockFileModificationTimes.remove(filePath); From d8d626d979b2010cfc1328ef78580443187efcae Mon Sep 17 00:00:00 2001 From: Danny Tuppeny Date: Wed, 22 Nov 2023 18:27:39 +0000 Subject: [PATCH 5/7] Revert bad update This would result in Remove events for files that don't still exist on the second poll --- lib/src/file_watcher/polling.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/src/file_watcher/polling.dart b/lib/src/file_watcher/polling.dart index 5787d53..a4400a4 100644 --- a/lib/src/file_watcher/polling.dart +++ b/lib/src/file_watcher/polling.dart @@ -61,7 +61,7 @@ class _PollingFileWatcher implements FileWatcher, ManuallyClosedWatcher { var pathExists = await File(path).exists(); if (_eventsController.isClosed) return; - if (!_isFirstPoll && !pathExists) { + if (_lastModified != null && !pathExists) { _eventsController.add(WatchEvent(ChangeType.REMOVE, path)); unawaited(close()); return; From 6095f6e9aa9a7d839c5bd24df482c6d05c98886d Mon Sep 17 00:00:00 2001 From: Danny Tuppeny Date: Wed, 22 Nov 2023 18:29:33 +0000 Subject: [PATCH 6/7] Remove redundant flag --- lib/src/file_watcher/polling.dart | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/lib/src/file_watcher/polling.dart b/lib/src/file_watcher/polling.dart index a4400a4..a3374b5 100644 --- a/lib/src/file_watcher/polling.dart +++ b/lib/src/file_watcher/polling.dart @@ -37,12 +37,6 @@ class _PollingFileWatcher implements FileWatcher, ManuallyClosedWatcher { /// The timer that controls polling. late final Timer _timer; - /// Track whether the next [_poll] is the first. - /// - /// We cannot just check [_lastModified] is null, because that is a valid - /// state for a file that does not exist. - var _isFirstPoll = true; - /// The previous modification time of the file. /// /// `null` indicates the file does not (or did not on the last poll) exist. @@ -78,12 +72,11 @@ class _PollingFileWatcher implements FileWatcher, ManuallyClosedWatcher { } if (_eventsController.isClosed) return; - if (_isFirstPoll) { + if (!isReady) { // If this is the first poll, don't emit an event, just set the last mtime // and complete the completer. _lastModified = modified; _readyCompleter.complete(); - _isFirstPoll = false; return; } From 4774d1697ba81cb4c798104672d0eee2b42df30f Mon Sep 17 00:00:00 2001 From: Danny Tuppeny Date: Wed, 22 Nov 2023 18:53:19 +0000 Subject: [PATCH 7/7] Ensure we already mark ourselves as ready --- lib/src/file_watcher/polling.dart | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/lib/src/file_watcher/polling.dart b/lib/src/file_watcher/polling.dart index a3374b5..15ff9ab 100644 --- a/lib/src/file_watcher/polling.dart +++ b/lib/src/file_watcher/polling.dart @@ -56,6 +56,7 @@ class _PollingFileWatcher implements FileWatcher, ManuallyClosedWatcher { if (_eventsController.isClosed) return; if (_lastModified != null && !pathExists) { + _flagReady(); _eventsController.add(WatchEvent(ChangeType.REMOVE, path)); unawaited(close()); return; @@ -66,17 +67,21 @@ class _PollingFileWatcher implements FileWatcher, ManuallyClosedWatcher { modified = await modificationTime(path); } on FileSystemException catch (error, stackTrace) { if (!_eventsController.isClosed) { + _flagReady(); _eventsController.addError(error, stackTrace); await close(); } } - if (_eventsController.isClosed) return; + if (_eventsController.isClosed) { + _flagReady(); + return; + } if (!isReady) { // If this is the first poll, don't emit an event, just set the last mtime // and complete the completer. _lastModified = modified; - _readyCompleter.complete(); + _flagReady(); return; } @@ -86,6 +91,13 @@ class _PollingFileWatcher implements FileWatcher, ManuallyClosedWatcher { _eventsController.add(WatchEvent(ChangeType.MODIFY, path)); } + /// Flags this watcher as ready if it has not already been done. + void _flagReady() { + if (!isReady) { + _readyCompleter.complete(); + } + } + @override Future close() async { _timer.cancel();