From ee8a268028801a61589c364858b99e33124de352 Mon Sep 17 00:00:00 2001 From: flutteractionsbot <154381524+flutteractionsbot@users.noreply.github.com> Date: Tue, 18 Nov 2025 07:14:05 -0800 Subject: [PATCH] [CP-beta][ Widget Preview ] Gracefully handle unexpected analysis context disposal (#178645) This pull request is created by [automatic cherry pick workflow](https://github.com/flutter/flutter/blob/main/docs/releases/Flutter-Cherrypick-Process.md#automatically-creates-a-cherry-pick-request) Please fill in the form below, and a flutter domain expert will evaluate this cherry pick request. ### Issue Link: What is the link to the issue this cherry-pick is addressing? https://github.com/flutter/flutter/issues/178472 ### Changelog Description: Explain this cherry pick in one line that is accessible to most Flutter developers. See [best practices](https://github.com/flutter/flutter/blob/main/docs/releases/Hotfix-Documentation-Best-Practices.md) for examples Widget preview command can crash on exit if in the middle of analyzing changes to a Dart file. ### Impact Description: What is the impact (ex. visual jank on Samsung phones, app crash, cannot ship an iOS app)? Does it impact development (ex. flutter doctor crashes when Android Studio is installed), or the shipping production app (the app crashes on launch) Unnecessary noise in our crash reporting. Shouldn't have a noticeable impact on development experience. ### Workaround: Is there a workaround for this issue? No. ### Risk: What is the risk level of this cherry-pick? ### Test Coverage: Are you confident that your fix is well-tested by automated tests? ### Validation Steps: What are the steps to validate that this fix works? 1. Run `flutter widget-preview start` in a Flutter project 2. Edit a source file and immediately send SIGQUIT to the `flutter` process 3. Repeat this process multiple times to verify no error is reported --- .../src/widget_preview/dependency_graph.dart | 13 +- .../src/widget_preview/preview_detector.dart | 136 ++++++++++-------- .../lib/src/widget_preview/utils.dart | 6 + ...eview_detector_regression_178472_test.dart | 99 +++++++++++++ 4 files changed, 190 insertions(+), 64 deletions(-) create mode 100644 packages/flutter_tools/test/commands.shard/permeable/widget_preview/preview_detector_regression_178472_test.dart 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(); +}