diff --git a/packages/flutter_tools/lib/src/widget_preview/dependency_graph.dart b/packages/flutter_tools/lib/src/widget_preview/dependency_graph.dart index 69d8950e664..4e11f470d5a 100644 --- a/packages/flutter_tools/lib/src/widget_preview/dependency_graph.dart +++ b/packages/flutter_tools/lib/src/widget_preview/dependency_graph.dart @@ -215,11 +215,14 @@ final class LibraryPreviewNode { Future populateErrors({required AnalysisContext context}) async { errors.clear(); for (final String file in files) { - errors.addAll( - ((await context.currentSession.getErrors(file)) as ErrorsResult).diagnostics - .where((error) => error.severity == Severity.error) - .toList(), - ); + final SomeErrorsResult errorsResult = await context.currentSession.getErrors(file); + // If errorsResult isn't an ErrorsResult, the analysis context has likely been disposed and + // we're in the process of shutting down. Ignore those results. + if (errorsResult is ErrorsResult) { + errors.addAll( + errorsResult.diagnostics.where((error) => error.severity == Severity.error).toList(), + ); + } } } diff --git a/packages/flutter_tools/lib/src/widget_preview/preview_detector.dart b/packages/flutter_tools/lib/src/widget_preview/preview_detector.dart index cb3c2e2735e..bf9dde46f6a 100644 --- a/packages/flutter_tools/lib/src/widget_preview/preview_detector.dart +++ b/packages/flutter_tools/lib/src/widget_preview/preview_detector.dart @@ -55,7 +55,8 @@ class PreviewDetector { static const kWindowsFileWatcherRestartedMessage = 'WindowsDirectoryWatcher has closed and been restarted.'; StreamSubscription? _fileWatcher; - final _mutex = PreviewDetectorMutex(); + @visibleForTesting + final mutex = PreviewDetectorMutex(); var _disposed = false; @@ -70,34 +71,36 @@ class PreviewDetector { /// Starts listening for changes to Dart sources under [projectRoot] and returns /// the initial [PreviewDependencyGraph] for the project. - Future initialize() async { - // Find the initial set of previews. - await _findPreviewFunctions(projectRoot); + Future initialize() { + return mutex.runGuarded(() async { + // Find the initial set of previews. + await findPreviewFunctions(projectRoot); - // Determine which files have transitive dependencies with compile time errors. - _propagateErrors(); + // Determine which files have transitive dependencies with compile time errors. + _propagateErrors(); - final Watcher watcher = watcherBuilder(projectRoot.path); - _fileWatcher = watcher.events.listen( - _onFileSystemEvent, - onError: (Object e, StackTrace st) { - if (platform.isWindows && - e is FileSystemException && - e.message.startsWith(kDirectoryWatcherClosedUnexpectedlyPrefix)) { - // The Windows directory watcher sometimes decides to shutdown on its own. It's - // automatically restarted by package:watcher, but we need to handle this exception. - // See https://github.com/dart-lang/tools/issues/1713 for details. - logger.printTrace(kWindowsFileWatcherRestartedMessage); - return; - } - Error.throwWithStackTrace(e, st); - }, - ); + final Watcher watcher = watcherBuilder(projectRoot.path); + _fileWatcher = watcher.events.listen( + _onFileSystemEvent, + onError: (Object e, StackTrace st) { + if (platform.isWindows && + e is FileSystemException && + e.message.startsWith(kDirectoryWatcherClosedUnexpectedlyPrefix)) { + // The Windows directory watcher sometimes decides to shutdown on its own. It's + // automatically restarted by package:watcher, but we need to handle this exception. + // See https://github.com/dart-lang/tools/issues/1713 for details. + logger.printTrace(kWindowsFileWatcherRestartedMessage); + return; + } + Error.throwWithStackTrace(e, st); + }, + ); - // Wait for file watcher to finish initializing, otherwise we might miss changes and cause - // tests to flake. - await watcher.ready; - return _dependencyGraph; + // Wait for file watcher to finish initializing, otherwise we might miss changes and cause + // tests to flake. + await watcher.ready; + return _dependencyGraph; + }); } Future dispose() async { @@ -107,7 +110,7 @@ class PreviewDetector { _disposed = true; // Guard disposal behind a mutex to make sure the analyzer has finished // processing the latest file updates to avoid throwing an exception. - await _mutex.runGuarded(() async { + await mutex.runGuarded(() async { await _fileWatcher?.cancel(); _fileWatcher = null; await collection.dispose(); @@ -117,7 +120,7 @@ class PreviewDetector { Future _onFileSystemEvent(WatchEvent event) async { // Only process one FileSystemEntity at a time so we don't invalidate an AnalysisSession that's // in use when we call context.changeFile(...). - await _mutex.runGuarded(() async { + await mutex.runGuarded(() async { final String eventPath = event.path; // Ignore any files under .dart_tool or ephemeral directories created by // the tool (e.g., build/, plugin directories, etc.). @@ -162,7 +165,13 @@ class PreviewDetector { // We need to notify the analyzer that this file has changed so it can reanalyze the file. final File file = fs.file(eventPath); context.changeFile(file.path); - final List potentiallyAffectedFiles = await context.applyPendingFileChanges(); + final List potentiallyAffectedFiles; + try { + potentiallyAffectedFiles = await context.applyPendingFileChanges(); + } on DisposedAnalysisContextResult { + // We're shutting down. + return; + } logger.printStatus('Detected change in $eventPath.'); if (event.type == ChangeType.REMOVE) { @@ -172,7 +181,7 @@ class PreviewDetector { } for (final filePath in potentiallyAffectedFiles) { - await _fileAddedOrUpdated(context: context, filePath: filePath); + await _fileAddedOrUpdated(filePath: filePath); } // TODO(bkonyi): If _fileAddedOrUpdated is called after _fileRemoved, it'll add the removed file back... @@ -189,11 +198,8 @@ class PreviewDetector { }); } - Future _fileAddedOrUpdated({ - required AnalysisContext context, - required String filePath, - }) async { - final PreviewDependencyGraph filePreviewsMapping = await _findPreviewFunctions( + Future _fileAddedOrUpdated({required String filePath}) async { + final PreviewDependencyGraph filePreviewsMapping = await findPreviewFunctions( fs.file(filePath), ); if (filePreviewsMapping.length > 1) { @@ -226,7 +232,9 @@ class PreviewDetector { } /// Search for functions annotated with `@Preview` in the current project. - Future _findPreviewFunctions(FileSystemEntity entity) async { + @visibleForTesting + Future findPreviewFunctions(FileSystemEntity entity) async { + assert(mutex.isLocked); final PreviewDependencyGraph updatedPreviews = PreviewDependencyGraph(); logger.printStatus('Finding previews in ${entity.path}...'); @@ -241,34 +249,43 @@ class PreviewDetector { // If filePath points to a file that's part of a library, retrieve its compilation unit first // in order to get the actual path to the library. if (lib is NotLibraryButPartResult) { - final unit = - (await context.currentSession.getResolvedUnit(filePath)) as ResolvedUnitResult; + final SomeResolvedUnitResult unit = await context.currentSession.getResolvedUnit( + filePath, + ); + // Check that unit is a valid response. Otherwise, the analysis context has likely been + // disposed or we're shutting down. + if (unit is! ResolvedUnitResult) { + continue; + } lib = await context.currentSession.getResolvedLibrary( unit.libraryElement.firstFragment.source.fullName, ); } - if (lib is ResolvedLibraryResult) { - final ResolvedLibraryResult resolvedLib = lib; - final PreviewPath previewPath = lib.element.toPreviewPath(); - // This library has already been processed. - if (updatedPreviews.containsKey(previewPath)) { - continue; - } - - final LibraryPreviewNode previewsForLibrary = _dependencyGraph.putIfAbsent( - previewPath, - () => LibraryPreviewNode(library: resolvedLib.element, logger: logger), - ); - - previewsForLibrary.updateDependencyGraph(graph: _dependencyGraph, units: lib.units); - updatedPreviews[previewPath] = previewsForLibrary; - - // Check for errors in the library. - await previewsForLibrary.populateErrors(context: context); - - // Iterate over each library's AST to find previews. - previewsForLibrary.findPreviews(lib: lib); + // Check that lib is a valid response. Otherwise, the analysis context has likely been + // disposed or we're shutting down. + if (lib is! ResolvedLibraryResult) { + continue; } + final ResolvedLibraryResult resolvedLib = lib; + final PreviewPath previewPath = lib.element.toPreviewPath(); + // This library has already been processed. + if (updatedPreviews.containsKey(previewPath)) { + continue; + } + + final LibraryPreviewNode previewsForLibrary = _dependencyGraph.putIfAbsent( + previewPath, + () => LibraryPreviewNode(library: resolvedLib.element, logger: logger), + ); + + previewsForLibrary.updateDependencyGraph(graph: _dependencyGraph, units: lib.units); + updatedPreviews[previewPath] = previewsForLibrary; + + // Check for errors in the library. + await previewsForLibrary.populateErrors(context: context); + + // Iterate over each library's AST to find previews. + previewsForLibrary.findPreviews(lib: lib); } } final int previewCount = updatedPreviews.values.fold( @@ -285,6 +302,7 @@ class PreviewDetector { /// as checking for newly introduced errors in files which had a transitive dependency on the /// removed file. Future _fileRemoved({required AnalysisContext context, required String filePath}) async { + assert(mutex.isLocked); final File file = fs.file(filePath); final LibraryPreviewNode? node = _dependencyGraph.values.firstWhereOrNull( (LibraryPreviewNode e) => e.files.contains(file.path), diff --git a/packages/flutter_tools/lib/src/widget_preview/utils.dart b/packages/flutter_tools/lib/src/widget_preview/utils.dart index 6b37a8904ca..60fe3398b47 100644 --- a/packages/flutter_tools/lib/src/widget_preview/utils.dart +++ b/packages/flutter_tools/lib/src/widget_preview/utils.dart @@ -125,6 +125,12 @@ class PreviewDetectorMutex { _locked = false; } + /// Returns true if the lock is currently held. + /// + /// WARNING: this should only be used for assertions in functions that expect + /// the mutex to already be held. + bool get isLocked => _locked; + var _locked = false; final _outstandingRequests = Queue>(); } diff --git a/packages/flutter_tools/test/commands.shard/permeable/widget_preview/preview_detector_regression_178472_test.dart b/packages/flutter_tools/test/commands.shard/permeable/widget_preview/preview_detector_regression_178472_test.dart new file mode 100644 index 00000000000..20e8f5d1839 --- /dev/null +++ b/packages/flutter_tools/test/commands.shard/permeable/widget_preview/preview_detector_regression_178472_test.dart @@ -0,0 +1,99 @@ +// Copyright 2014 The Flutter Authors. 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:async'; + +import 'package:file/memory.dart'; +import 'package:flutter_tools/src/base/file_system.dart'; +import 'package:flutter_tools/src/base/logger.dart'; +import 'package:flutter_tools/src/base/platform.dart'; +import 'package:flutter_tools/src/project.dart'; +import 'package:flutter_tools/src/widget_preview/analytics.dart'; +import 'package:flutter_tools/src/widget_preview/dependency_graph.dart'; +import 'package:flutter_tools/src/widget_preview/preview_detector.dart'; +import 'package:test/fake.dart'; +import 'package:watcher/watcher.dart'; + +import '../../../src/common.dart'; +import '../../../src/context.dart'; +import '../../../src/fakes.dart'; + +void main() { + group('$PreviewDetector regression test https://github.com/flutter/flutter/issues/178472 -', () { + late LocalFileSystem fs; + late FlutterProject project; + late PreviewDetector previewDetector; + late FakeWatcher watcher; + late BufferLogger logger; + + setUp(() { + fs = LocalFileSystem.test(signals: FakeSignals()); + watcher = FakeWatcher(); + logger = BufferLogger.test(); + project = FlutterProject.fromDirectoryTest(fs.systemTempDirectory.createTempSync('root')); + previewDetector = PreviewDetector( + projectRoot: project.directory, + platform: FakePlatform(), + previewAnalytics: WidgetPreviewAnalytics( + analytics: getInitializedFakeAnalyticsInstance( + fakeFlutterVersion: FakeFlutterVersion(), + // We don't care about analytics in this test, so don't worry about having to + // provide a local file system. + fs: MemoryFileSystem.test(), + ), + ), + logger: logger, + fs: fs, + onChangeDetected: (_) {}, + onPubspecChangeDetected: (_) {}, + watcherBuilder: (_) => watcher, + ); + }); + + tearDown(() async { + // Don't explicitly tear down the previewDetector as we've already disposed + // the underlying analysis context collection. If we try and dispose it again, + // we'll hang. + await watcher.close(); + }); + + test('do not throw when watch event is sent after the analysis context is disposed', () async { + final File file = project.directory.childDirectory('lib').childFile('foo.dart') + ..createSync(recursive: true); + final String filePath = file.path; + await previewDetector.initialize(); + await previewDetector.collection.dispose(); + watcher.controller.add(WatchEvent(ChangeType.ADD, filePath)); + }); + + test( + 'do not throw when findPreviewFunctions is invoked after the analysis context is disposed', + () async { + final File file = project.directory.childDirectory('lib').childFile('foo.dart') + ..createSync(recursive: true); + await previewDetector.initialize(); + await previewDetector.collection.dispose(); + final PreviewDependencyGraph result = await previewDetector.mutex.runGuarded( + () => previewDetector.findPreviewFunctions(file), + ); + expect(result.entries, isEmpty); + }, + ); + }); +} + +class FakeWatcher extends Fake implements Watcher { + final controller = StreamController(); + + @override + Stream get events => controller.stream; + + @override + bool get isReady => true; + + @override + Future get ready => Future.value(); + + Future close() => controller.close(); +}