ProxiedDevice, connection to a remotely connected device via flutter daemon. #95738

chingjun · 2021-12-23T17:48:06Z

Also refactored DaemonStreams (that I wrote in a previous PR) for better reusability and testability, and added the ability to allow daemon commands to pass binary streams.

In my local tests passing a 100MB file in binary stream is ~50% faster than including a base64-encoded version in JSON.

Context: b/210724354

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.

chingjun · 2022-01-05T16:01:45Z

Rebased this PR now that #95290 has been submitted.

Also reactored DaemonStreams (that I wrote in a previous PR) for better reusability and testability, and added the ability to allow daemon commands to pass binary streams.

In my local tests passing a 100MB file in binary stream is ~50% faster than including a base64-encoded version in JSON.

PTAL, thanks!

jmagman

Minor nits. This review is huge. 🙂

/cc @zanderso @christopherfujino

jmagman · 2022-01-12T01:42:08Z

packages/flutter_tools/lib/src/daemon.dart

Lower camel https://dart.dev/guides/language/effective-dart/style#prefer-using-lowercamelcase-for-constant-names

Suggested change

JSON,

BINARY,

json,

binary,

Note: We initially used Java’s SCREAMING_CAPS style for constants. We changed for a few reasons:
SCREAMING_CAPS looks bad for many cases, particularly enum values for things like CSS colors.
Constants are often changed to final non-const variables, which would necessitate a name change.
The values property automatically defined on an enum type is const and lowercase.

Done, changed to lower case

jmagman · 2022-01-12T01:43:42Z

packages/flutter_tools/lib/src/daemon.dart

Maybe handle error case where message is malformed?

I preserved the original behavior for backward compatibility. In the original daemon, anything that is not JSON string are simply ignored.

jmagman · 2022-01-12T01:48:04Z

packages/flutter_tools/lib/src/device.dart

Suggested change

// final BuildInfo buildInfo;

Removed, thanks for noticing this :)

zanderso · 2022-01-12T16:20:50Z

packages/flutter_tools/lib/src/commands/daemon.dart

Consider adding a comment here explaining what this id is for.

Comment added

zanderso · 2022-01-12T16:26:55Z

packages/flutter_tools/lib/src/commands/daemon.dart

Consider adding a comment here. The comment should probably answer the question, "What's the difference between getDevices() and discoverDevices()?"

zanderso · 2022-01-12T16:32:31Z

packages/flutter_tools/lib/src/commands/daemon.dart

nit: else is unnecessary.

Removed the else

zanderso · 2022-01-12T16:32:47Z

packages/flutter_tools/lib/src/commands/daemon.dart

nit: else is unnecessary.

Removed the else

zanderso · 2022-01-12T16:36:25Z

packages/flutter_tools/lib/src/commands/daemon.dart

The binary parameter is only used in a few out of many instances of this callback type. Are there any drawbacks to using two callback tables for the two different callback types to avoid passing the binary parameter to many places it isn't used?

This is a great idea! Just added a new callback table. Also added assertion so that the same method will not be registered on both callback tables. Thanks!

zanderso · 2022-01-12T16:52:28Z

packages/flutter_tools/lib/src/proxied_devices/devices.dart

This seems like a programming error rather than an error that an end-user would be expected to address. In that case, this should be a normal exception so that it will hit crash logging and/or the end-user will see it as unusual and file a bug.

Changed to throw Exception.

zanderso · 2022-01-12T16:53:24Z

packages/flutter_tools/lib/src/proxied_devices/devices.dart

commented code.

zanderso · 2022-01-12T16:59:44Z

packages/flutter_tools/lib/src/proxied_devices/devices.dart

Consider adding an error handling callback for the server socket.

Added an error handler that prints the error with a logger to help diagnosing issues.

zanderso · 2022-01-12T17:02:43Z

packages/flutter_tools/test/commands.shard/hermetic/daemon_test.dart

Not a blocker, but I'm curious to know whether these tests touch the global context. Could they be testWithoutContext?

The Daemon class in commands/daemon.dart is using the global context in several places, it errors out when I tried to use testWithoutContext.

zanderso · 2022-01-12T17:04:10Z

packages/flutter_tools/test/general.shard/daemon_test.dart

Can these be testWithoutContext?

Done, changed to testWithoutContext

chingjun · 2022-01-19T18:30:30Z

Thanks for the extensive review Jenn, Zech and Chris! I've pushed a new commit based on the feedback, the biggest change is that I've refactored the convertInputStream async* function into a separate class and managed to remove the async* by using a StreamController instead. PTAL, thanks!

christopherfujino · 2022-01-19T20:48:54Z

packages/flutter_tools/lib/src/commands/daemon.dart

should this not be:

assert(!_handlers.containsKey(name));

My intention is to check that the same method name is not registered in both command map.

But I think it doesn't harm to check both maps, and less error prone too. I'll update to check both maps.

christopherfujino · 2022-01-19T21:03:17Z

packages/flutter_tools/lib/src/commands/daemon.dart

avoid throwing strings. is this an internal state error, or user error? Use a throwToolExit('device "$deviceId" not found'); if this is a user error.

I was following the other throw style in the same file. Should I change all the throw in this file to throwToolExit instead? If so can I do that in a follow-up PR?

Haven't looked at this code in awhile. I wonder if the strings get caught and sent as an error response back to the client. Even if that's the case though, we should wrap in a class to make that intent explicit at the point of the throw.

chingjun · 2022-01-21T23:52:16Z

Friendly ping @zanderso if you want to take another look at this? Thanks!

…daemon.

Also refactor DaemonStreams for better reusability and testability.

…flutter daemon. (flutter/flutter#95738)

zanderso

Sorry for the slow review.

This change has low test-coverage of error cases. I think even after the error handling that has already been added, and the couple of places that I've suggested in the new round of comments, I'm not confident that all cases are covered. Sockets and streams in Dart are difficult to reason about, so we'll need you to be available to fix crashers in this new code as we discover them from crash logging data. If you are not signing up for that, we will have to back out this change, and reland only with much better test coverage of error cases.

zanderso · 2022-01-26T16:18:14Z

packages/flutter_tools/lib/src/commands/daemon.dart

Haven't looked at this code in awhile. I wonder if the strings get caught and sent as an error response back to the client. Even if that's the case though, we should wrap in a class to make that intent explicit at the point of the throw.

zanderso · 2022-01-26T16:26:24Z

packages/flutter_tools/lib/src/commands/daemon.dart

+  Future<String> connect(Map<String, dynamic> args) async {
+    final int targetPort = _getIntArg(args, 'port', required: true);
+    final String id = 'portForwarder_${targetPort}_${_id++}';
+    final Socket socket = await Socket.connect('127.0.0.1', targetPort);


Instead of '127.0.0.1', this should use InternetAddress.loopbackIPv4 and fall back on InternetAddress.loopbackIPv6 if that doesn't work. This call can also throw a SocketException, which should be handled here if it shouldn't propagate like that.

zanderso · 2022-01-26T16:28:32Z

packages/flutter_tools/lib/src/commands/daemon.dart

+    final String id = 'portForwarder_${targetPort}_${_id++}';
+    final Socket socket = await Socket.connect('127.0.0.1', targetPort);
+    _forwardedConnections[id] = socket;
+    socket.listen((List<int> data) {


You need an error handler on this listen() and possibly also an error handler on the done future or socket exceptions on remote disconnect may crash the tool.

zanderso · 2022-01-26T16:30:33Z

packages/flutter_tools/lib/src/commands/daemon.dart

+  @override
+  Future<void> dispose() async {
+    for (final Socket connection in _forwardedConnections.values) {
+      await connection.close();


Do you want destroy here? https://api.dart.dev/dev/2.17.0-63.0.dev/dart-io/Socket/destroy.html

zanderso · 2022-01-26T16:38:02Z

packages/flutter_tools/lib/src/daemon.dart

+
+    int start = 0;
+    while (start < chunk.length) {
+      if (state == _InputStreamParseState.json) {


Since the state is in fields instead of locals, you can pull the handling of these cases out into helper methods.

zanderso · 2022-01-26T16:43:08Z

packages/flutter_tools/lib/src/daemon.dart

-  Future<void> dispose() async {
-    await (await _socket).close();
+  /// Creates a [DaemonStreams] that uses stdin and stdout as the underlying streams.
+  static DaemonStreams fromStdio(Stdio stdio, { required Logger logger }) {


Dart style here would be to use a named forwarding factory constructor:

DaemonStreams.fromStdio(Stdio stdio, {required Logger logger}) : this(stdio.stdin, stdio.stdout, logger: logger);

zanderso · 2022-01-26T16:44:00Z

packages/flutter_tools/lib/src/daemon.dart

+  }
+
+  /// Creates a [DaemonStreams] that uses [Socket] as the underlying streams.
+  static DaemonStreams fromSocket(Socket socket, { required Logger logger }) {


Ditto:

DaemonStreams.fromSocket(Socket socket, { required Logger logger }) : this(socket, socket, logger: logger);

zanderso · 2022-01-26T16:45:19Z

packages/flutter_tools/lib/src/daemon.dart

+  }
+
+  /// Connects to a server and creates a [DaemonStreams] from the connection as the underlying streams.
+  static DaemonStreams connect(String host, int port, { required Logger logger }) {


And this should be a factory constructor:

factory DaemonStreams.connect(String host, int port, {required Logger logger}) { ... }

zanderso · 2022-01-26T16:46:41Z

packages/flutter_tools/lib/src/daemon.dart

+    final Future<Socket> socketFuture = Socket.connect(host, port);
+    final StreamController<List<int>> inputStreamController = StreamController<List<int>>();
+    final StreamController<List<int>> outputStreamController = StreamController<List<int>>();
+    socketFuture.then((Socket socket) {


The socket future needs an error handler.

zanderso · 2022-01-26T16:50:16Z

packages/flutter_tools/lib/src/device.dart

  @override
  String toString() => value;
+
+  static Category? fromString(String category) {


Here and below, using a const map lookup can help readability:

static Category? fromString(String category) { return <String, Category>{ 'web': web, ... }[category]; }

chingjun · 2022-01-26T21:00:24Z

Sorry about that. The changes here will mostly not be used by default (especially the parts related to Sockets), and are only used in google3, so I think most of the changes here should be relatively safe for 3p users. The changes are submitted upstream only because it might be useful for some advanced 3p users.

And yes, I can fix upcoming issues or crashes if they come up, and I will send a separate PR to address the feedbacks you sent above.

Thanks.

…flutter daemon. (flutter/flutter#95738)

…daemon. (flutter#95738) Also allow daemon commands to pass binary streams

chingjun requested review from christopherfujino and jmagman December 23, 2021 17:48

flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Dec 23, 2021

chingjun force-pushed the proxied-device branch from 8e48b39 to 434b41c Compare December 23, 2021 18:54

chingjun force-pushed the proxied-device branch from 434b41c to 81666d0 Compare January 5, 2022 15:57

jmagman approved these changes Jan 12, 2022

View reviewed changes

zanderso reviewed Jan 12, 2022

View reviewed changes

christopherfujino reviewed Jan 19, 2022

View reviewed changes

chingjun force-pushed the proxied-device branch 2 times, most recently from 8498d31 to ad68ea7 Compare January 21, 2022 23:11

chingjun added 4 commits January 25, 2022 22:01

ProxiedDevice, connection to a remotely connected device via flutter …

44a97a4

…daemon.

Allow daemon commands to pass binary streams

ec633b0

Also refactor DaemonStreams for better reusability and testability.

Update according to review feedback

15a2721

Add assertion that prevents handlers from being added twice

74313d8

chingjun force-pushed the proxied-device branch from ad68ea7 to 74313d8 Compare January 26, 2022 06:01

chingjun merged commit 1065826 into flutter:master Jan 26, 2022

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

1065826 ProxiedDevice, connection to a remotely connected device via …

c52cb20

…flutter daemon. (flutter/flutter#95738)

engine-flutter-autoroll mentioned this pull request Jan 26, 2022

Roll Flutter from f6ee92589274 to 10658266866d (10 revisions) flutter/packages#557

Merged

zanderso reviewed Jan 26, 2022

View reviewed changes

chingjun mentioned this pull request Jan 26, 2022

Change all instance of throwing strings to throw specific error classes. #97325

Merged

8 tasks

chingjun added a commit to chingjun/flutter that referenced this pull request Jan 29, 2022

Addresses the feedbacks in flutter#95738

4a433e8

fluttergithubbot pushed a commit that referenced this pull request Feb 2, 2022

Addresses the feedbacks in #95738 (#97457)

a504b93

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

1065826 ProxiedDevice, connection to a remotely connected device via …

78efe22

…flutter daemon. (flutter/flutter#95738)

clocksmith pushed a commit to clocksmith/flutter that referenced this pull request Mar 8, 2022

ProxiedDevice, connection to a remotely connected device via flutter …

1b8f9b1

…daemon. (flutter#95738) Also allow daemon commands to pass binary streams

clocksmith pushed a commit to clocksmith/flutter that referenced this pull request Mar 8, 2022

Addresses the feedbacks in flutter#95738 (flutter#97457)

1393dad

-  JSON,
-  BINARY,
+  json,
+  binary,

ProxiedDevice, connection to a remotely connected device via flutter daemon. #95738

ProxiedDevice, connection to a remotely connected device via flutter daemon. #95738

Uh oh!

Conversation

chingjun commented Dec 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

chingjun commented Jan 5, 2022

Uh oh!

jmagman left a comment

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!

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!

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!

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!

chingjun commented Jan 19, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christopherfujino Jan 19, 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!

chingjun commented Jan 21, 2022

chingjun commented Dec 23, 2021 •

edited

Loading

christopherfujino Jan 19, 2022 •

edited

Loading

zanderso left a comment •

edited

Loading