Add an option for flutter daemon to listen on a TCP port #95418

chingjun · 2021-12-16T19:02:36Z

Added a new class DaemonConnection to reuse the connection handling
between daemon server and client, and handle connection with different
medium (stdio, socket).

Added a new option listen-on-tcp-port to the flutter daemon command,
when passed, the daemon will accept commands on a port instead of stdio.

This will be used to create a device proxy.

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.

Added a new class DaemonConnection to reuse the connection handling between daemon server and client, and handle connection with different medium (stdio, socket). Added a new option `listen-on-tcp-port` to the flutter daemon command, when passed, the daemon will accept commands on a port instead of stdio.

jmagman · 2021-12-21T20:09:02Z

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

+      final String port = stringArg('listen-on-tcp-port');
+      await _DaemonServer(
+        port: int.parse(port),


This should probably be checked that it's a valid int.

Suggested change

final String port = stringArg('listen-on-tcp-port');

await _DaemonServer(

port: int.parse(port),

int port;

try {

port = int.parse(stringArg('listen-on-tcp-port'));

} on FormatException catch (error) {

throwToolExit('Invalid port for `--listen-on-tcp-port`: $error');

}

await _DaemonServer(

port: port,

jmagman · 2021-12-21T20:10:12Z

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

+    this.notifyingLogger,
+  });
+
+  int port;


Suggested change

int port;

final int port;

jmagman · 2021-12-21T20:10:21Z

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

+  int port;
+
+  /// Stdout logger used to print general server-related errors.
+  Logger logger;


Suggested change

Logger logger;

final Logger logger;

jmagman · 2021-12-21T20:10:32Z

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

+  Logger logger;
+
+  // Logger that sends the message to the other end of daemon connection.
+  NotifyingLogger notifyingLogger;


Suggested change

NotifyingLogger notifyingLogger;

final NotifyingLogger notifyingLogger;

jmagman · 2021-12-21T20:13:19Z

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

        if (!_onExitCompleter.isCompleted) {
          _onExitCompleter.complete(0);
        }
      },
    );
  }

+  DaemonConnection connection;


Suggested change

DaemonConnection connection;

final DaemonConnection connection;

jmagman · 2021-12-21T20:39:54Z

packages/flutter_tools/lib/src/daemon.dart

+  void send(Map<String, Object?> message) {
+    globals.stdio.stdoutWrite(
+      '[${json.encode(message)}]\n',
+      fallback: (String message, dynamic error, StackTrace stack) {


Suggested change

fallback: (String message, dynamic error, StackTrace stack) {

fallback: (String message, Object? error, StackTrace stack) {

packages/flutter_tools/lib/src/daemon.dart

jmagman · 2021-12-21T20:46:31Z

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

-      map['params'] = _toJsonable(args);
-    }
-    _send(map);
+    daemon.connection.sendEvent(name, args != null ? _toJsonable(args) : null);


_toJsonable returns null when args is null.

Suggested change

daemon.connection.sendEvent(name, args != null ? _toJsonable(args) : null);

daemon.connection.sendEvent(name, _toJsonable(args));

jmagman · 2021-12-21T20:55:43Z

packages/flutter_tools/lib/src/daemon.dart

+}
+
+/// A [DaemonStream] that uses [Socket] as the underlying stream.
+class TcpDaemonStreams extends DaemonStreams {


Is this tested anywhere?

Added a new test for this class

jmagman · 2021-12-21T20:57:15Z

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

+/// microtask queue. This avoids a deadlock when tests `await` a Future
+/// which queues a microtask that will not be processed unless the queue
+/// is flushed.
+// Future<T> _runFakeAsync<T>(Future<T> Function(FakeAsync time) f) async {


chingjun · 2021-12-21T22:10:40Z

Thanks for the thorough review! I've just pushed the fix for the review feedbacks. PTAL, thanks!

jmagman

Minor nits

jmagman · 2021-12-21T22:15:20Z

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

-          stdinCommandStream,
-          stdoutCommandResponse,
+          DaemonConnection(
+            daemonStreams: StdioDaemonStreams(),


Suggested change

daemonStreams: StdioDaemonStreams(),

daemonStreams: StdioDaemonStreams(globals.stdio),

jmagman · 2021-12-21T22:15:36Z

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

    globals.printStatus('Starting device daemon...');
    final Daemon daemon = Daemon(
-      stdinCommandStream, stdoutCommandResponse,
+      DaemonConnection(
+        daemonStreams: StdioDaemonStreams(),


Suggested change

daemonStreams: StdioDaemonStreams(),

daemonStreams: StdioDaemonStreams(globals.stdio),

jmagman · 2021-12-21T22:15:54Z

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

-        stdinCommandStream,
-        stdoutCommandResponse,
+        DaemonConnection(
+          daemonStreams: StdioDaemonStreams(),


Suggested change

daemonStreams: StdioDaemonStreams(),

daemonStreams: StdioDaemonStreams(globals.stdio),

jmagman · 2021-12-21T22:16:23Z

packages/flutter_tools/lib/src/daemon.dart

+  StdioDaemonStreams([Stdio? stdio]) :
+    _stdio = stdio ?? globals.stdio,
+    inputStream = _convertInputStream((stdio ?? globals.stdio).stdin);


Make the injected globals required, ideally we should be able to remove import 'globals.dart' as globals; from almost everywhere but context_runner.dart. Unfortunately the commands don't have their dependencies injected yet, so the global context objects will need to be passed from there.

Suggested change

StdioDaemonStreams([Stdio? stdio]) :

_stdio = stdio ?? globals.stdio,

inputStream = _convertInputStream((stdio ?? globals.stdio).stdin);

StdioDaemonStreams(Stdio stdio) :

_stdio = stdio,

inputStream = _convertInputStream(stdio.stdin);

jmagman · 2021-12-21T22:17:11Z

packages/flutter_tools/lib/src/daemon.dart

+    Logger? logger,
+  }): _logger = logger ?? globals.logger {


Suggested change

Logger? logger,

}): _logger = logger ?? globals.logger {

required Logger logger,

}): _logger = logger {

jmagman · 2021-12-21T22:17:34Z

packages/flutter_tools/lib/src/daemon.dart

+    Logger? logger,
+  }) : _logger = logger ?? globals.logger {


Suggested change

Logger? logger,

}) : _logger = logger ?? globals.logger {

required Logger logger,

}) : _logger = logger {

jmagman · 2021-12-21T22:17:59Z

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

+        });
+        final Daemon daemon = Daemon(
+          DaemonConnection(
+            daemonStreams: TcpDaemonStreams(socket),


Suggested change

daemonStreams: TcpDaemonStreams(socket),

daemonStreams: TcpDaemonStreams(socket, logger: logger),

jmagman · 2021-12-21T22:24:06Z

packages/flutter_tools/lib/src/daemon.dart

+  }
+
+  /// Connects to a remote host and creates a [DaemonStreams] from the socket.
+  TcpDaemonStreams.connect(


Is this used?

This will be used in my next PR, would you want me to remove it from here, and add the change in my next PR instead?

No, you can leave it in this PR, wanted to make sure it wasn't leftover from something else you tried.

jmagman · 2021-12-21T22:26:23Z

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

+
+// @dart = 2.8


Suggested change

// @dart = 2.8

Make this file null safe.

jmagman · 2021-12-21T22:27:02Z

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

+  BufferLogger bufferLogger;
+  FakeDaemonStreams daemonStreams;
+  DaemonConnection daemonConnection;


Suggested change

BufferLogger bufferLogger;

FakeDaemonStreams daemonStreams;

DaemonConnection daemonConnection;

late BufferLogger bufferLogger;

late FakeDaemonStreams daemonStreams;

late DaemonConnection daemonConnection;

chingjun · 2021-12-21T23:25:29Z

Just pushed the changes for the review feedback, PTAL, thanks!

jmagman

LGTM

chingjun · 2021-12-22T14:20:55Z

Thanks for the review Jenn!

)" This reverts commit 2b46ea4.

)" (#95686) This reverts commit 2b46ea4.

…tter#95418)" Fixes the test brekage and reverts commit caed03d.

)" (#95689) Fixes the test brekage and reverts commit caed03d.

chingjun requested review from jmagman and christopherfujino December 16, 2021 19:02

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

chingjun force-pushed the daemon-socket branch from 317d3f5 to 07d852d Compare December 16, 2021 19:09

chingjun force-pushed the daemon-socket branch from 07d852d to 759e9cf Compare December 16, 2021 19:31

jmagman reviewed Dec 21, 2021

View reviewed changes

Review feedback and add test for TcpDaemonStreams

4a36116

jmagman reviewed Dec 21, 2021

View reviewed changes

Review feedbacks

72c8429

jmagman approved these changes Dec 22, 2021

View reviewed changes

chingjun merged commit 2b46ea4 into flutter:master Dec 22, 2021

zanderso added a commit that referenced this pull request Dec 22, 2021

Revert "Add an option for flutter daemon to listen on a TCP port (#95418

d608e43

)" This reverts commit 2b46ea4.

zanderso mentioned this pull request Dec 22, 2021

Revert "Add an option for flutter daemon to listen on a TCP port" #95686

Merged

zanderso added a commit that referenced this pull request Dec 22, 2021

Revert "Add an option for flutter daemon to listen on a TCP port (#95418

caed03d

)" (#95686) This reverts commit 2b46ea4.

chingjun added a commit to chingjun/flutter that referenced this pull request Dec 22, 2021

Reland "Add an option for flutter daemon to listen on a TCP port (flu…

56cc5cb

…tter#95418)" Fixes the test brekage and reverts commit caed03d.

chingjun added a commit that referenced this pull request Dec 22, 2021

Reland "Add an option for flutter daemon to listen on a TCP port (#95418

df384c4

)" (#95689) Fixes the test brekage and reverts commit caed03d.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an option for flutter daemon to listen on a TCP port #95418

Add an option for flutter daemon to listen on a TCP port #95418

chingjun commented Dec 16, 2021

jmagman Dec 21, 2021

jmagman Dec 21, 2021

jmagman Dec 21, 2021

jmagman Dec 21, 2021

jmagman Dec 21, 2021

jmagman Dec 21, 2021

jmagman Dec 21, 2021

jmagman Dec 21, 2021

chingjun Dec 21, 2021

jmagman Dec 21, 2021

chingjun commented Dec 21, 2021

jmagman left a comment

jmagman Dec 21, 2021

jmagman Dec 21, 2021

jmagman Dec 21, 2021

jmagman Dec 21, 2021

jmagman Dec 21, 2021

jmagman Dec 21, 2021

jmagman Dec 21, 2021

jmagman Dec 21, 2021

chingjun Dec 21, 2021

jmagman Dec 22, 2021

jmagman Dec 21, 2021

jmagman Dec 21, 2021

chingjun commented Dec 21, 2021

jmagman left a comment

chingjun commented Dec 22, 2021

	NotifyingLogger notifyingLogger;
	final NotifyingLogger notifyingLogger;

	DaemonConnection connection;
	final DaemonConnection connection;

	fallback: (String message, dynamic error, StackTrace stack) {
	fallback: (String message, Object? error, StackTrace stack) {

	daemon.connection.sendEvent(name, args != null ? _toJsonable(args) : null);
	daemon.connection.sendEvent(name, _toJsonable(args));

	daemonStreams: StdioDaemonStreams(),
	daemonStreams: StdioDaemonStreams(globals.stdio),

	daemonStreams: TcpDaemonStreams(socket),
	daemonStreams: TcpDaemonStreams(socket, logger: logger),

Add an option for flutter daemon to listen on a TCP port #95418

Add an option for flutter daemon to listen on a TCP port #95418

Conversation

chingjun commented Dec 16, 2021

Pre-launch Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chingjun commented Dec 21, 2021

jmagman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chingjun commented Dec 21, 2021

jmagman left a comment

Choose a reason for hiding this comment

chingjun commented Dec 22, 2021