Repository navigation
Revert: [flutter_tools] Fix DTD "Stream already subscribed" error and wait for analysis over DTD in widget preview - #193637
Conversation
… wait for analysis over DTD in widget preview (flutter#192930)" This reverts commit bcb21e8.
There was a problem hiding this comment.
Code Review
This pull request refactors the widget preview tool's integration with the Dart Tooling Daemon (DTD) and the LSP service. Key changes include checking the DTD Service stream for LSP service registration, introducing retry logic for preview RPC calls to handle temporary 'method not found' errors, and streamlining the analysis server startup logic. Review feedback identifies a potential resource leak and RPC exception risk in _waitForLspService due to a missing streamCancel call and lack of concurrent call deduplication. Additionally, refactoring the retry loops in getFlutterWidgetPreviews and getFlutterWidgetPreviewsForFile is recommended to eliminate unreachable dead code.
| Future<void> _waitForLspService() async { | ||
| if (_allLspServicesRegistered) { | ||
| if (_lspServiceAvailable) { | ||
| return; | ||
| } | ||
| final Future<void>? future = _waitForLspServiceFuture; | ||
| if (future != null) { | ||
| return future; | ||
| } | ||
| _waitForLspServiceFuture = _waitForLspServiceHelper(); | ||
| try { | ||
| await _waitForLspServiceFuture; | ||
| } finally { | ||
| _waitForLspServiceFuture = null; | ||
| } | ||
| } | ||
|
|
||
| bool _areRequiredLspServicesRegistered(RegisteredServicesResponse registeredServices) { | ||
| return registeredServices.clientServices.any( | ||
| (service) => | ||
| service.name == kLspStream && kRequiredLspServices.every(service.methods.containsKey), | ||
| ); | ||
| } | ||
|
|
||
| Future<void> _waitForLspServiceHelper() async { | ||
| final lspInitializedCompleter = Completer<void>(); | ||
| final lspRegisteredCompleter = Completer<void>(); | ||
|
|
||
| final StreamSubscription<DTDEvent> lspSubscription = _dtd!.onEvent(kLspStream).listen(( | ||
| const kServiceStream = 'Service'; | ||
| await _dtd!.streamListen(kServiceStream); | ||
| final StreamSubscription<DTDEvent> serviceSubscription = _dtd!.onEvent(kServiceStream).listen(( | ||
| DTDEvent event, | ||
| ) { | ||
| if (lspInitializedCompleter.isCompleted) { | ||
| if (lspRegisteredCompleter.isCompleted) { | ||
| return; | ||
| } | ||
| if (event.kind == kLspInitializedEvent) { | ||
| _allLspServicesRegistered = true; | ||
| lspInitializedCompleter.complete(); | ||
| if (event case DTDEvent(kind: 'ServiceRegistered', data: {'service': kLspStream})) { | ||
| _lspServiceAvailable = true; | ||
| lspRegisteredCompleter.complete(); | ||
| } | ||
| }); | ||
|
|
||
| try { | ||
| await _dtd!.safeStreamListen(kLspStream); | ||
| final RegisteredServicesResponse registeredServices = await _dtd!.getRegisteredServices(); | ||
| if (_areRequiredLspServicesRegistered(registeredServices)) { | ||
| _allLspServicesRegistered = true; | ||
| if (!lspInitializedCompleter.isCompleted) { | ||
| lspInitializedCompleter.complete(); | ||
| } | ||
| final bool alreadyRegistered = | ||
| registeredServices.dtdServices.contains(kLspStream) || | ||
| registeredServices.clientServices.any((service) => service.name == kLspStream); | ||
| if (alreadyRegistered) { | ||
| _lspServiceAvailable = true; | ||
| lspRegisteredCompleter.complete(); | ||
| } else { | ||
| logger.printStatus('Waiting for analysis server to register Lsp service with DTD...'); | ||
| await lspInitializedCompleter.future.timeout(const Duration(seconds: 30)); | ||
| await lspRegisteredCompleter.future.timeout(const Duration(seconds: 30)); | ||
| } | ||
| } on TimeoutException { | ||
| logger.printWarning('Timed out waiting for the Lsp service to be registered with DTD.'); | ||
| rethrow; | ||
| } finally { | ||
| await lspSubscription.cancel(); | ||
| await serviceSubscription.cancel(); | ||
| } | ||
| } |
There was a problem hiding this comment.
When _waitForLspService is called, it subscribes to the Service stream using streamListen. However, if the operation times out or fails, the stream subscription on the DTD side is never cancelled because streamCancel is not called. This leads to a resource leak on the DTD server and will cause subsequent calls to _waitForLspService to fail with a Stream already subscribed (code 103) RPC exception.
Additionally, without deduplication of concurrent calls to _waitForLspService, multiple concurrent requests will attempt to call streamListen simultaneously, triggering the same Stream already subscribed error.
Consider wrapping the stream subscription in a way that ensures streamCancel is called in the finally block, and deduplicating concurrent calls using a shared Future.
Future<void>? _waitForLspServiceFuture;
Future<void> _waitForLspService() async {
if (_lspServiceAvailable) {
return;
}
final Future<void>? future = _waitForLspServiceFuture;
if (future != null) {
return future;
}
_waitForLspServiceFuture = _waitForLspServiceHelper();
try {
await _waitForLspServiceFuture;
} finally {
_waitForLspServiceFuture = null;
}
}
Future<void> _waitForLspServiceHelper() async {
final lspRegisteredCompleter = Completer<void>();
const kServiceStream = 'Service';
await _dtd!.streamListen(kServiceStream);
final StreamSubscription<DTDEvent> serviceSubscription = _dtd!.onEvent(kServiceStream).listen((
DTDEvent event,
) {
if (lspRegisteredCompleter.isCompleted) {
return;
}
if (event case DTDEvent(kind: 'ServiceRegistered', data: {'service': kLspStream})) {
_lspServiceAvailable = true;
lspRegisteredCompleter.complete();
}
});
try {
final RegisteredServicesResponse registeredServices = await _dtd!.getRegisteredServices();
final bool alreadyRegistered =
registeredServices.dtdServices.contains(kLspStream) ||
registeredServices.clientServices.any((service) => service.name == kLspStream);
if (alreadyRegistered) {
_lspServiceAvailable = true;
lspRegisteredCompleter.complete();
} else {
logger.printStatus('Waiting for analysis server to register Lsp service with DTD...');
await lspRegisteredCompleter.future.timeout(const Duration(seconds: 30));
}
} on TimeoutException {
logger.printWarning('Timed out waiting for the Lsp service to be registered with DTD.');
rethrow;
} finally {
await serviceSubscription.cancel();
try {
await _dtd!.streamCancel(kServiceStream);
} on Exception {
// ignore
}
}
}| const maxAttempts = 50; | ||
| for (var attempts = 0; attempts < maxAttempts; attempts++) { | ||
| try { | ||
| final DTDResponse result = await _dtd!.call( | ||
| 'Lsp', | ||
| 'dart/workspace/getFlutterWidgetPreviews', | ||
| ); | ||
| return FlutterWidgetPreviews.fromJson(result.result['result']! as Map<String, Object?>); | ||
| } on RpcException catch (e) { | ||
| if (e.code == -32601 && attempts < maxAttempts - 1) { | ||
| // Method not found | ||
| await Future<void>.delayed(const Duration(milliseconds: 200)); | ||
| continue; | ||
| } | ||
| rethrow; | ||
| } | ||
| } | ||
| throw StateError('Failed to call getFlutterWidgetPreviews after $maxAttempts attempts.'); |
There was a problem hiding this comment.
The retry loop uses a fixed for loop with a trailing throw StateError to satisfy the analyzer's definite return analysis. However, because the last iteration of the loop always rethrows the RpcException, the trailing throw StateError is unreachable dead code.
We can refactor this to use a while (true) loop with an explicit counter. This is cleaner, avoids unreachable code, and is naturally understood by the Dart analyzer as non-completing normally.
| const maxAttempts = 50; | |
| for (var attempts = 0; attempts < maxAttempts; attempts++) { | |
| try { | |
| final DTDResponse result = await _dtd!.call( | |
| 'Lsp', | |
| 'dart/workspace/getFlutterWidgetPreviews', | |
| ); | |
| return FlutterWidgetPreviews.fromJson(result.result['result']! as Map<String, Object?>); | |
| } on RpcException catch (e) { | |
| if (e.code == -32601 && attempts < maxAttempts - 1) { | |
| // Method not found | |
| await Future<void>.delayed(const Duration(milliseconds: 200)); | |
| continue; | |
| } | |
| rethrow; | |
| } | |
| } | |
| throw StateError('Failed to call getFlutterWidgetPreviews after $maxAttempts attempts.'); | |
| var attempts = 0; | |
| while (true) { | |
| try { | |
| final DTDResponse result = await _dtd!.call( | |
| 'Lsp', | |
| 'dart/workspace/getFlutterWidgetPreviews', | |
| ); | |
| return FlutterWidgetPreviews.fromJson(result.result['result']! as Map<String, Object?>); | |
| } on RpcException catch (e) { | |
| if (e.code == -32601 && attempts < 49) { | |
| attempts++; | |
| await Future<void>.delayed(const Duration(milliseconds: 200)); | |
| continue; | |
| } | |
| rethrow; | |
| } | |
| } |
| const maxAttempts = 50; | ||
| for (var attempts = 0; attempts < maxAttempts; attempts++) { | ||
| try { | ||
| final DTDResponse result = await _dtd!.call( | ||
| 'Lsp', | ||
| 'dart/textDocument/getFlutterWidgetPreviews', | ||
| params: {'uri': Uri.file(filePath).toString()}, | ||
| ); | ||
| return FlutterWidgetPreviews.fromJson(result.result['result']! as Map<String, Object?>); | ||
| } on RpcException catch (e) { | ||
| if (e.code == -32601 && attempts < maxAttempts - 1) { | ||
| // Method not found | ||
| await Future<void>.delayed(const Duration(milliseconds: 200)); | ||
| continue; | ||
| } | ||
| rethrow; | ||
| } | ||
| } | ||
| throw StateError('Failed to call getFlutterWidgetPreviewsForFile after $maxAttempts attempts.'); |
There was a problem hiding this comment.
Similarly, we can refactor this retry loop to use a while (true) loop with an explicit counter to eliminate the unreachable trailing throw StateError.
| const maxAttempts = 50; | |
| for (var attempts = 0; attempts < maxAttempts; attempts++) { | |
| try { | |
| final DTDResponse result = await _dtd!.call( | |
| 'Lsp', | |
| 'dart/textDocument/getFlutterWidgetPreviews', | |
| params: {'uri': Uri.file(filePath).toString()}, | |
| ); | |
| return FlutterWidgetPreviews.fromJson(result.result['result']! as Map<String, Object?>); | |
| } on RpcException catch (e) { | |
| if (e.code == -32601 && attempts < maxAttempts - 1) { | |
| // Method not found | |
| await Future<void>.delayed(const Duration(milliseconds: 200)); | |
| continue; | |
| } | |
| rethrow; | |
| } | |
| } | |
| throw StateError('Failed to call getFlutterWidgetPreviewsForFile after $maxAttempts attempts.'); | |
| var attempts = 0; | |
| while (true) { | |
| try { | |
| final DTDResponse result = await _dtd!.call( | |
| 'Lsp', | |
| 'dart/textDocument/getFlutterWidgetPreviews', | |
| params: {'uri': Uri.file(filePath).toString()}, | |
| ); | |
| return FlutterWidgetPreviews.fromJson(result.result['result']! as Map<String, Object?>); | |
| } on RpcException catch (e) { | |
| if (e.code == -32601 && attempts < 49) { | |
| attempts++; | |
| await Future<void>.delayed(const Duration(milliseconds: 200)); | |
| continue; | |
| } | |
| rethrow; | |
| } | |
| } |
|
Detected the If you add the The "Merge" button is also unlocked. To bypass presubmits as well as the tree status, press the GitHub "Add to Merge Queue". |
…13104) Manual roll requested by [email protected] flutter/flutter@d649d2b...e89fd0a 2026-10-01 [email protected] Manual roll ICU from 6ebb40c59477 to 5aa526207171 (2 revisions) (flutter/flutter#193661) 2026-10-01 [email protected] fix(web): hash source maps before rewriting sourceMappingURL with --web-content-hash (flutter/flutter#193614) 2026-10-01 [email protected] [flutter_tools] Bootstrap DeviceManager in ToolDependencies (flutter/flutter#193524) 2026-10-01 [email protected] [wasm] Set flutter_runtime_mode in tools/gn to_gn_wasm_args (flutter/flutter#193407) 2026-10-01 [email protected] Remove remaining `download_jdk` references (flutter/flutter#193562) 2026-10-01 [email protected] Synchronize dart2wasm feature probes in roll script and flutter_bootstrap.js (flutter/flutter#193271) 2026-10-01 [email protected] [web] Unskip web tests that now pass in gestures, services, and widgets (flutter/flutter#193512) 2026-10-01 [email protected] Roll Fuchsia Test Scripts from whZAV1mfT-Rdc0rQ8... to fCNknap4ZI0AMcvIf... (flutter/flutter#193640) 2026-10-01 [email protected] Roll Skia from 0575a37d0619 to f2d68e0b8863 (2 revisions) (flutter/flutter#193621) 2026-10-01 [email protected] perf(widgets): use const <Never>{} in InheritedModelElement and avoid redundant casts in RenderObjectElement.update (flutter/flutter#192913) 2026-10-01 [email protected] [web] Unskip and strengthen missing glyph tofu test on Wimp (flutter/flutter#193426) 2026-10-01 [email protected] [flutter_tools] Fix `flutter drive -d chrome` hanging on web (flutter/flutter#193459) 2026-10-01 [email protected] [web] Fix hang when Chromium cannot bind remote debugging port (flutter/flutter#193461) 2026-10-01 [email protected] [flutter_tools] Bundle data assets from hooks in flutter test (flutter/flutter#193435) 2026-10-01 [email protected] Roll Packages from 0ba9a82 to d5ec6db (11 revisions) (flutter/flutter#193641) 2026-10-01 [email protected] Revert: [flutter_tools] Fix DTD "Stream already subscribed" error and wait for analysis over DTD in widget preview (flutter/flutter#193637) 2026-10-01 [email protected] Roll Skia from 6182a3cf8f98 to 0575a37d0619 (5 revisions) (flutter/flutter#193613) 2026-10-01 [email protected] [flutter_tools] Fix DTD "Stream already subscribed" error and wait for analysis over DTD in widget preview (flutter/flutter#192930) 2026-09-30 [email protected] [Impeller] Sample the render target rect in offscreen advanced blends (flutter/flutter#192988) 2026-09-30 [email protected] Roll Skia from 01524014dff5 to 6182a3cf8f98 (11 revisions) (flutter/flutter#193594) 2026-09-30 [email protected] [web] Use Xvfb -displayfd and clean up Xvfb after Firefox test suites (flutter/flutter#193510) 2026-09-30 [email protected] Fixes bug where sibling node fail to update size (flutter/flutter#191220) 2026-09-30 [email protected] Roll Skia from c7b323126bf0 to 01524014dff5 (3 revisions) (flutter/flutter#193577) 2026-09-30 [email protected] Remove some Undefined Behavior from run_customer_testing_tests.dart. (flutter/flutter#193582) 2026-09-30 [email protected] Fix pointer id/coordinate mismatch in Android platform view multi-touch (flutter/flutter#191631) 2026-09-30 [email protected] Roll Packages from 3c6ce59 to 0ba9a82 (11 revisions) (flutter/flutter#193578) 2026-09-30 [email protected] Fix unresolved doc comment references in dev/ tooling (flutter/flutter#193501) If this roll has caused a breakage, revert this CL and set the roller to dry run mode using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
… wait for analysis over DTD in widget preview" (flutter#193658) ## Description Relands flutter#192930 (which was reverted in flutter#193637). ### What changed since the original PR - In `AnalysisServer`, store and cancel the `stdout` and `stderr` stream subscriptions in `dispose()` before closing `_analyzingController` and `_errorsController`, and guard `_analyzingController.add` with `!_analyzingController.isClosed` in `_handleProgress`. Previously, `$/progress` notifications arriving from the language server during or after `AnalysisServer.dispose()` threw `Bad state: Cannot add new events after calling close` in `widget_preview_scaffold_test.dart`. - In `widget_preview_test_helpers.dart`, await `analysisServer.onExit` after `analysisServer.dispose()` in `addTearDown` so the analysis server process has fully exited before temporary directory cleanup on Windows. - Added a hermetic unit test in `analyze_continuously_test.dart` verifying that `AnalysisServer` does not throw when `$/progress` notifications arrive during or after `dispose()`. ### Original description During widget preview startup or file updates: 1. Concurrent calls to retrieve previews or wait for analysis (e.g., initial command startup racing with file watcher events) could concurrently invoke `_waitForLspService()`, causing duplicate `streamListen` calls to the Dart Tooling Daemon. DTD throws JSON-RPC error 103 (`Stream already subscribed`) when a client attempts to subscribe to a stream it is already listening to. 2. `WidgetPreviewDtdServices` previously considered the `Lsp` service ready as soon as *any* `Lsp` method was registered on DTD (or upon receiving the first `ServiceRegistered` event for `Lsp`). Because the analysis server registers its LSP methods sequentially before posting an `initialized` event on the `Lsp` stream, calls to `Lsp.dart/workspace/analysis/complete` or `Lsp.dart/workspace/getFlutterWidgetPreviews` could race ahead of their registration and fail with `-32601 (method not found)`. 3. When connecting to a pre-existing analysis server over DTD (`--dtd-url`), `LspPreviewDetector.analysisServer` is `null`, causing `analysisServer?.waitForAnalysis()` calls during startup and file updates to be no-ops. This change: - Deduplicates in-flight calls to `_waitForLspService()` using a shared future (`_waitForLspServiceFuture`) so concurrent callers await the same LSP registration check instead of issuing duplicate DTD requests. - Separates `lspServiceAvailable` (whether an analysis server has begun registering `Lsp` services on DTD) from `_allLspServicesRegistered` (whether all required widget preview LSP methods in `kRequiredLspServices` are registered), and updates `_waitForLspServiceHelper()` to listen for the `initialized` event on the `Lsp` stream before checking `getRegisteredServices()`. - Consolidates LSP DTD calls into `_callLspService()` and removes the `kMethodNotFound` polling loop now that `_waitForLspService()` ensures all required LSP methods are registered. - Safely catches and ignores `RpcErrorCodes.kStreamAlreadySubscribed` on `streamListen` calls in `WidgetPreviewDtdServices` (`safeStreamListen`). - Adds `WidgetPreviewDtdServices.waitForAnalysis()` (invoking `Lsp.dart/workspace/analysis/complete` over DTD), unifies `LspPreviewDetector.waitForAnalysis()` so widget preview waits for analysis completion on both tool-managed and pre-existing analysis servers, and moves the startup `waitForAnalysis()` call inside the DTD error-handling `try / catch` block in `WidgetPreviewStartCommand`. - Adds hermetic regression tests covering concurrent/pre-subscribed DTD streams, partial vs. full LSP service registration (`Lsp.initialized`), and waiting for analysis over DTD. Fixes flutter#192424 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant in-code documentation (doc comments with `///`). - [x] If this PR introduces a new feature or capability, I created and linked a website documentation issue or PR in [flutter/website] (or verified none is needed). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [AI contribution guidelines]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [flutter/website]: https://github.com/flutter/website [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
Reverts: [flutter_tools] Fix DTD "Stream already subscribed" error and wait for analysis over DTD in widget preview
Initiated by: @jtmcdole
Reason for reverting: ```
Original PR Author: @bkonyi
Reviewed By: @DanTup
The original PR description is provided below:
Description
During widget preview startup or file updates:
_waitForLspService(), causing duplicatestreamListencalls to the Dart Tooling Daemon. DTD throws JSON-RPC error 103 (Stream already subscribed) when a client attempts to subscribe to a stream it is already listening to.WidgetPreviewDtdServicespreviously considered theLspservice ready as soon as anyLspmethod was registered on DTD (or upon receiving the firstServiceRegisteredevent forLsp). Because the analysis server registers its LSP methods sequentially before posting aninitializedevent on theLspstream, calls toLsp.dart/workspace/analysis/completeorLsp.dart/workspace/getFlutterWidgetPreviewscould race ahead of their registration and fail with-32601 (method not found).--dtd-url),LspPreviewDetector.analysisServerisnull, causinganalysisServer?.waitForAnalysis()calls during startup and file updates to be no-ops.This change:
_waitForLspService()using a shared future (_waitForLspServiceFuture) so concurrent callers await the same LSP registration check instead of issuing duplicate DTD requests.lspServiceAvailable(whether an analysis server has begun registeringLspservices on DTD) from_allLspServicesRegistered(whether all required widget preview LSP methods inkRequiredLspServicesare registered), and updates_waitForLspServiceHelper()to listen for theinitializedevent on theLspstream before checkinggetRegisteredServices()._callLspService()and removes thekMethodNotFoundpolling loop now that_waitForLspService()ensures all required LSP methods are registered.RpcErrorCodes.kStreamAlreadySubscribedonstreamListencalls inWidgetPreviewDtdServices(safeStreamListen).WidgetPreviewDtdServices.waitForAnalysis()(invokingLsp.dart/workspace/analysis/completeover DTD), unifiesLspPreviewDetector.waitForAnalysis()so widget preview waits for analysis completion on both tool-managed and pre-existing analysis servers, and moves the startupwaitForAnalysis()call inside the DTD error-handlingtry / catchblock inWidgetPreviewStartCommand.Lsp.initialized), and waiting for analysis over DTD.Fixes #192424
Pre-launch Checklist
///).