[flutter_tools/dap] Add support for forwarding `flutter run --machine` exposeUrl requests to the DAP client #114539

DanTup · 2022-11-02T18:48:03Z

This adds support for the app.exposeUrl functionality of the flutter run --machine daemon to the DAPs to support some types of remote workspaces.

The functionality already exists in the Flutter daemon and the original Dart-Code DAP, but was not reproduced here.

The way this works is:

IDE runs flutter run --machine with the --allow-expose-url flag
When Flutter runs a web server (for the app, or the DWDS proxy), it sends an app.exposeUrl request back to the client asking it to make the URL (usually localhost with some port) available to the users machine, and return the URL that should be used (for example when running in Docker, it would expose the port in docker and return the URL with the port number swapped... when running in a cloud IDE, it would set up forwarding through a proxy and map localhost to something like my-app-port-8080.some_long_guid.cloud-ide.foo)
Flutter would then use that URL when it needs to provide the URL in a context that will be used by the end user (for example when opening a browser tab or injecting DWDS metadata into the app)

So this change adds support for forwarding requests from stdout of flutter run --machine over to the client (via a custom event) and accepts their response (via a custom request) and sends it back to Flutter. Right now, app.exposeUrl is the only daemon-to-editor request (see https://github.com/flutter/flutter/blob/master/packages/flutter_tools/doc/daemon.md#daemon-to-editor-requests) but this code is mostly generic so if others are added in future they will only need adding to _requestsToForwardToClient.

In the test for this, we play the part of the client, by accepting the forwarded request and just changing the host to mapped-host and then verify that a message is sent back to Flutter with that new URL.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

…` requests to the DAP client Currently the only request that Flutter sends to the client is `app.exposeUrl` though most of this code is generic to support other requests that may be added in future.

christopherfujino · 2022-11-07T17:36:35Z

packages/flutter_tools/lib/src/debug_adapters/flutter_adapter.dart

+    final String messageString = jsonEncode(message);
+    // Flutter requests are always wrapped in brackets as an array.
+    final String payload = '[$messageString]\n';
+    process?.stdin.writeln(payload);


what does it mean if process == null?

process is null when the adapter is first started, until it has spawned flutter run or flutter attach in response to a launchRequest or attachRequest. Generally we won't get here when that's the case, although there may be cases where it's more convenient to use this as a "send if the process exists" such as trying to clean it up/shut it down. I suspect it'd be fine to change to a ! though.

Should we make process a Future<process>? or at least print an error if we drop a request? or am I overthinking it?

Making it a Future<Process> would make sense if we wanted to chain this work on the end. However probably if the user asks us to quit before we started the process, we want to just quit without spawning the process instead.

We could print something (where "print" here means "send an OutputEvent to the client") although it's probably just a junk message to the user - if they've clicked Stop on a debug session they likely just expect everything to shut down and showing messages that we couldn't send messages to a process we hadn't yet started is probably not that useful.

My feeling is that this is probably an error case and if we do end up here when it's null, we would treat it as a bug. Although, crashing the debug adapter with an exception versus doing nothing is probably an unfortunate way to handle it (because it's very likely we're just shutting down and silently dropping it would provide the expected user). I think it's very unlikely to happen though (the ? is mostly there because Dart requires it), so if using ! and triggering an error is the way we'd normally handle this sort of unexpected condition (or an assert so it only happens in tests), we should probably do that?

I changed this slightly.. I instead added a throw DartDebugAdapterException() if it's null (which also avoids the need for ? or !). I added this because I found some paths where a client could trigger this (for example if they send a hot reload immediately after sending launchRequest before we had started the process). This type of exception is already handled and reports back to the user "Failed to Hot Restart: [message]" so this works out.

I also tweaked the Mock to bypass calling this method since it was relying on process = null being valid and we don't actually spawn a process in tests (instead we just capture the messages that we would've sent to stdin so they can be verified).

(I'm not sure if Google Testing has gotten stuck?)

christopherfujino · 2022-11-07T18:08:33Z

packages/flutter_tools/lib/src/debug_adapters/flutter_adapter.dart

+        eventType: 'flutter.forwardedRequest',
+      );
+    } else {
+      completer.completeError('Unknown request method "$method".');


I think this is eventually going to be an analysis error for only_throw_errors (right now the lint is only enforced for errors thrown with the throw keyword). Any reason not to wrap this in an exception?

christopherfujino · 2022-11-07T18:09:05Z

packages/flutter_tools/lib/src/debug_adapters/flutter_adapter.dart

+    final Object? error = args?.args['error'];
+    final Completer<Object?>? completer = _reverseRequestCompleters[id];
+    if (error != null) {
+      completer?.completeError(error);


ditto about only_throw_errors

christopherfujino · 2022-11-07T18:13:24Z

packages/flutter_tools/test/general.shard/dap/flutter_adapter_test.dart

+        });
+
+        // Allow the handler to be processed.
+        await pumpEventQueue(times: 5000);


cool, didn't know about pumpEventQueue(). However, why 5000 times?

It was pretty arbitrary.. The default is like 20, but I was worried that might be racy if the handler code happens to await a few times. The analysis server tests use 5000 in a lot of places so I've just always assumed that's a reasonable number (and that the cost of this is very low but the cost of investigating flakes might not be).

I don't know if this is a good reason though, so I'm happy to change - or even remove if makes sense.

Ahh, ok. My main concern was that this code path had 4983 asynchronous suspensions, and a later refactor might break this test. This is fine.

…ss starts

…no process

christopherfujino

LGTM, thanks!

…-machine` exposeUrl requests to the DAP client (flutter/flutter#114539)

* 3a656b1 Add more supported simulator debugging options and improve tests (flutter/flutter#114628) * 51c517c [flutter_tools/dap] Add support for forwarding `flutter run --machine` exposeUrl requests to the DAP client (flutter/flutter#114539) * 8e5439c Roll Flutter Engine from e7d7edab98ad to c76035429c36 (14 revisions) (flutter/flutter#115008) * a479718 Update cirrus key (flutter/flutter#115006) * dccc761 Roll Flutter Engine from c76035429c36 to aa4b3ea2f733 (27 revisions) (flutter/flutter#115025) * d0491dc Add the `channel` parameter to the Dartpad samples (flutter/flutter#115018) * befd8b6 722414a26 Implemented threadsafe platform channel replies on windows (flutter/engine#36909) (flutter/flutter#115033) * 154ae0f `updateSemantics` in TestWindow should always be implemented. (flutter/flutter#114857) * 008ac17 remove unnecessary brace in string interpolation (flutter/flutter#115032) * 8926d9e a9fbf6af5 Roll Fuchsia Linux SDK from mDzQK4ZUk_Y4wfZa_... to RNSA2Wp1MObtc7OHy... (flutter/engine#37485) (flutter/flutter#115045) * e6fb124 Roll Flutter Engine from a9fbf6af52c9 to b42ed6933cef (2 revisions) (flutter/flutter#115048) * ff75451 a42b2d9b9 Fix a race in the EmbedderA11yTest.A11yTreeIsConsistent tests (flutter/engine#37488) (flutter/flutter#115060) * c7d1154 a71c0c6c3 [Impeller] Support access the descriptor of PipelineFuture directly (flutter/engine#37415) (flutter/flutter#115063)

…-machine` exposeUrl requests to the DAP client (flutter/flutter#114539)

* 3a656b1 Add more supported simulator debugging options and improve tests (flutter/flutter#114628) * 51c517c [flutter_tools/dap] Add support for forwarding `flutter run --machine` exposeUrl requests to the DAP client (flutter/flutter#114539) * 8e5439c Roll Flutter Engine from e7d7edab98ad to c76035429c36 (14 revisions) (flutter/flutter#115008) * a479718 Update cirrus key (flutter/flutter#115006) * dccc761 Roll Flutter Engine from c76035429c36 to aa4b3ea2f733 (27 revisions) (flutter/flutter#115025) * d0491dc Add the `channel` parameter to the Dartpad samples (flutter/flutter#115018) * befd8b6 722414a26 Implemented threadsafe platform channel replies on windows (flutter/engine#36909) (flutter/flutter#115033) * 154ae0f `updateSemantics` in TestWindow should always be implemented. (flutter/flutter#114857) * 008ac17 remove unnecessary brace in string interpolation (flutter/flutter#115032) * 8926d9e a9fbf6af5 Roll Fuchsia Linux SDK from mDzQK4ZUk_Y4wfZa_... to RNSA2Wp1MObtc7OHy... (flutter/engine#37485) (flutter/flutter#115045) * e6fb124 Roll Flutter Engine from a9fbf6af52c9 to b42ed6933cef (2 revisions) (flutter/flutter#115048) * ff75451 a42b2d9b9 Fix a race in the EmbedderA11yTest.A11yTreeIsConsistent tests (flutter/engine#37488) (flutter/flutter#115060) * c7d1154 a71c0c6c3 [Impeller] Support access the descriptor of PipelineFuture directly (flutter/engine#37415) (flutter/flutter#115063)

…` exposeUrl requests to the DAP client (flutter#114539) * [flutter_tools/dap] Add support for forwarding `flutter run --machine` requests to the DAP client Currently the only request that Flutter sends to the client is `app.exposeUrl` though most of this code is generic to support other requests that may be added in future. * Improve comment * Fix thrown strings * StateError -> DebugAdapterException * Add a non-null assertion and assert * Use DebugAdapterException to handle restartRequests sent before process starts * Fix typo + use local var * Don't try to actually send Flutter messages in tests because there's no process

* 3a656b1 Add more supported simulator debugging options and improve tests (flutter/flutter#114628) * 51c517c [flutter_tools/dap] Add support for forwarding `flutter run --machine` exposeUrl requests to the DAP client (flutter/flutter#114539) * 8e5439c Roll Flutter Engine from e7d7edab98ad to c76035429c36 (14 revisions) (flutter/flutter#115008) * a479718 Update cirrus key (flutter/flutter#115006) * dccc761 Roll Flutter Engine from c76035429c36 to aa4b3ea2f733 (27 revisions) (flutter/flutter#115025) * d0491dc Add the `channel` parameter to the Dartpad samples (flutter/flutter#115018) * befd8b6 722414a26 Implemented threadsafe platform channel replies on windows (flutter/engine#36909) (flutter/flutter#115033) * 154ae0f `updateSemantics` in TestWindow should always be implemented. (flutter/flutter#114857) * 008ac17 remove unnecessary brace in string interpolation (flutter/flutter#115032) * 8926d9e a9fbf6af5 Roll Fuchsia Linux SDK from mDzQK4ZUk_Y4wfZa_... to RNSA2Wp1MObtc7OHy... (flutter/engine#37485) (flutter/flutter#115045) * e6fb124 Roll Flutter Engine from a9fbf6af52c9 to b42ed6933cef (2 revisions) (flutter/flutter#115048) * ff75451 a42b2d9b9 Fix a race in the EmbedderA11yTest.A11yTreeIsConsistent tests (flutter/engine#37488) (flutter/flutter#115060) * c7d1154 a71c0c6c3 [Impeller] Support access the descriptor of PipelineFuture directly (flutter/engine#37415) (flutter/flutter#115063)

DanTup requested a review from christopherfujino November 2, 2022 18:48

flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Nov 2, 2022

DanTup added 2 commits November 4, 2022 16:07

[flutter_tools/dap] Add support for forwarding `flutter run --machine…

b2738c8

…` requests to the DAP client Currently the only request that Flutter sends to the client is `app.exposeUrl` though most of this code is generic to support other requests that may be added in future.

Improve comment

b4252a4

DanTup force-pushed the expose-url branch from f90722c to b4252a4 Compare November 4, 2022 16:08

DanTup changed the title ~~[flutter_tools/dap] Add support for forwarding flutter run --machine requests to the DAP client~~ [flutter_tools/dap] Add support for forwarding flutter run --machine exposeUrl requests to the DAP client Nov 7, 2022

christopherfujino reviewed Nov 7, 2022

View reviewed changes

DanTup added 6 commits November 7, 2022 18:57

Fix thrown strings

e3d0834

StateError -> DebugAdapterException

047c82c

Add a non-null assertion and assert

8b8acf3

Use DebugAdapterException to handle restartRequests sent before proce…

47f817d

…ss starts

Fix typo + use local var

af4a31b

Don't try to actually send Flutter messages in tests because there's …

6800d0f

…no process

christopherfujino approved these changes Nov 9, 2022

View reviewed changes

DanTup merged commit 51c517c into flutter:master Nov 9, 2022

DanTup deleted the expose-url branch November 9, 2022 19:16

This was referenced Nov 10, 2022

Roll Flutter from d3dcd7d532b6 to c7d11549a51e (13 revisions) flutter/plugins#6689

Merged

Roll Flutter from d3dcd7d532b6 to c7d11549a51e (13 revisions) flutter/packages#2794

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Nov 10, 2022

51c517c [flutter_tools/dap] Add support for forwarding `flutter run -…

4c9c48c

…-machine` exposeUrl requests to the DAP client (flutter/flutter#114539)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 10, 2022

51c517c [flutter_tools/dap] Add support for forwarding `flutter run -…

274cff1

…-machine` exposeUrl requests to the DAP client (flutter/flutter#114539)

engine-flutter-autoroll mentioned this pull request Nov 10, 2022

Roll Flutter from d3dcd7d532b6 to c7d11549a51e (13 revisions) flutter/packages#2795

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 10, 2022

51c517c [flutter_tools/dap] Add support for forwarding `flutter run -…

4a24868

…-machine` exposeUrl requests to the DAP client (flutter/flutter#114539)

engine-flutter-autoroll mentioned this pull request Nov 10, 2022

Roll Flutter from d3dcd7d532b6 to bdcacbf8e3fd (14 revisions) flutter/packages#2796

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 10, 2022

51c517c [flutter_tools/dap] Add support for forwarding `flutter run -…

eb99169

…-machine` exposeUrl requests to the DAP client (flutter/flutter#114539)

engine-flutter-autoroll mentioned this pull request Nov 10, 2022

Roll Flutter from d3dcd7d532b6 to ca6165244f70 (15 revisions) flutter/packages#2798

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 10, 2022

51c517c [flutter_tools/dap] Add support for forwarding `flutter run -…

877d79b

…-machine` exposeUrl requests to the DAP client (flutter/flutter#114539)

engine-flutter-autoroll mentioned this pull request Nov 10, 2022

Roll Flutter from d3dcd7d532b6 to b31b9dc05f99 (16 revisions) flutter/packages#2799

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 10, 2022

51c517c [flutter_tools/dap] Add support for forwarding `flutter run -…

55c5d3a

…-machine` exposeUrl requests to the DAP client (flutter/flutter#114539)

This was referenced Nov 30, 2022

Web URLs are not exposed (app.exposeUrl) when using new SDK DAPs Dart-Code/Dart-Code#4291

Closed

[flutter_tools] Forward app.webLaunchUrl event from Flutter to DAP clients #116275

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[flutter_tools/dap] Add support for forwarding `flutter run --machine` exposeUrl requests to the DAP client #114539

[flutter_tools/dap] Add support for forwarding `flutter run --machine` exposeUrl requests to the DAP client #114539

Uh oh!

DanTup commented Nov 2, 2022

Uh oh!

christopherfujino Nov 7, 2022

Uh oh!

DanTup Nov 7, 2022

Uh oh!

christopherfujino Nov 7, 2022

Uh oh!

DanTup Nov 7, 2022

Uh oh!

christopherfujino Nov 7, 2022

Uh oh!

DanTup Nov 8, 2022

Uh oh!

DanTup Nov 8, 2022

Uh oh!

christopherfujino Nov 7, 2022 •

edited

Loading

Uh oh!

DanTup Nov 7, 2022

Uh oh!

christopherfujino Nov 7, 2022

Uh oh!

DanTup Nov 7, 2022

Uh oh!

christopherfujino Nov 7, 2022

Uh oh!

DanTup Nov 7, 2022

Uh oh!

christopherfujino Nov 7, 2022

Uh oh!

christopherfujino left a comment

Uh oh!

Uh oh!

[flutter_tools/dap] Add support for forwarding flutter run --machine exposeUrl requests to the DAP client #114539

[flutter_tools/dap] Add support for forwarding flutter run --machine exposeUrl requests to the DAP client #114539

Uh oh!

Conversation

DanTup commented Nov 2, 2022

Pre-launch Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christopherfujino Nov 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christopherfujino left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

[flutter_tools/dap] Add support for forwarding `flutter run --machine` exposeUrl requests to the DAP client #114539

[flutter_tools/dap] Add support for forwarding `flutter run --machine` exposeUrl requests to the DAP client #114539

christopherfujino Nov 7, 2022 •

edited

Loading