Add `--machine` support for `flutter attach` (#19077)

* Extract some of startApp into a reusable method

* Get basic attach --machine working

* Attach --machine tweaks

Move validation to validate method and create daemon early so we get the startup event before trying to get a connection.

* Bump daemon version so we know whether it's valid to flutter attach

* Tweak output text

* Swap package imports for relative

* Review tweaks (naming, formatting, typedefs)

* Separate arguments from process spawning

This will make calling attach easier

* Add a basic test for flutter attach --machine

* Fix crash if port unforward modifies the list of forwarded ports

* Add a no-op port forwarder for flutter-tester

* Switch to using BasicProject instead of our own inline code

* Fix expectation in test now we have a portForwarder

* Remove stale TODO (this is done)

* Tweak formatting

* Change some Completers to void to fix Dart 2 issues

packages/flutter_tools/doc/daemon.md

@@ -10,7 +10,7 @@ flutter daemon
 
 It runs a persistent, JSON-RPC based server to communicate with devices. IDEs and other tools can start the flutter tool in this mode and get device addition and removal notifications, as well as being able to programmatically start and stop apps on those devices.
 
-A set of `flutter daemon` commands/events are also exposed via `flutter run --machine` which allows IDEs and tools to launch flutter applications and interact to send commands like Hot Reload. The command and events that are available in this mode are documented at the bottom of this document.
+A set of `flutter daemon` commands/events are also exposed via `flutter run --machine` and `flutter attach --machine` which allow IDEs and tools to launch and attach to flutter applications and interact to send commands like Hot Reload. The command and events that are available in these modes are documented at the bottom of this document.
 
 ## Protocol
 
@@ -182,9 +182,9 @@ The returned `params` will contain:
 - `emulatorName` - the name of the emulator created; this will have been auto-generated if you did not supply one
 - `error` - when `success`=`false`, a message explaining why the creation of the emulator failed
 
-## Flutter Run --machine
+## 'flutter run --machine' and 'flutter attach --machine' 
 
-When running `flutter run --machine` the following subset of the daemon is available:
+When running `flutter run --machine` or `flutter attach --machine` the following subset of the daemon is available:
 
 ### daemon domain
 
@@ -219,5 +219,6 @@ See the [source](https://github.com/flutter/flutter/blob/master/packages/flutter
 
 ## Changelog
 
+- 0.4.1: Added `flutter attach --machine`
 - 0.4.0: Added `emulator.create` command
 - 0.3.0: Added `daemon.connected` event at startup

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

@@ -5,8 +5,10 @@
 import 'dart:async';
 
 import '../base/common.dart';
+import '../base/file_system.dart';
 import '../base/io.dart';
 import '../cache.dart';
+import '../commands/daemon.dart';
 import '../device.dart';
 import '../globals.dart';
 import '../protocol_discovery.dart';
@@ -35,16 +37,22 @@ final String ipv4Loopback = InternetAddress.loopbackIPv4.address;
 class AttachCommand extends FlutterCommand {
   AttachCommand({bool verboseHelp = false}) {
     addBuildModeFlags(defaultToRelease: false);
-    argParser.addOption(
+    argParser
+      ..addOption(
         'debug-port',
         help: 'Local port where the observatory is listening.',
-    );
-    argParser.addFlag(
-      'preview-dart-2',
-      defaultsTo: true,
-      hide: !verboseHelp,
-      help: 'Preview Dart 2.0 functionality.',
-    );
+      )
+      ..addFlag(
+        'preview-dart-2',
+        defaultsTo: true,
+        hide: !verboseHelp,
+        help: 'Preview Dart 2.0 functionality.',
+      )..addFlag('machine',
+          hide: !verboseHelp,
+          negatable: false,
+          help: 'Handle machine structured JSON command input and provide output\n'
+                'and progress in machine friendly format.',
+      );
   }
 
   @override
@@ -64,6 +72,14 @@ class AttachCommand extends FlutterCommand {
     return null;
   }
 
+  @override
+  Future<Null> validateCommand() async {
+    super.validateCommand();
+    if (await findTargetDevice() == null)
+      throwToolExit(null);
+    observatoryPort;
+  }
+
   @override
   Future<Null> runCommand() async {
     Cache.releaseLockEarly();
@@ -71,17 +87,24 @@ class AttachCommand extends FlutterCommand {
     await _validateArguments();
 
     final Device device = await findTargetDevice();
-    if (device == null)
-      throwToolExit(null);
     final int devicePort = observatoryPort;
+
+    final Daemon daemon = argResults['machine']
+      ? new Daemon(stdinCommandStream, stdoutCommandResponse,
+            notifyingLogger: new NotifyingLogger(), logToStdout: true)
+      : null;
+
     Uri observatoryUri;
     if (devicePort == null) {
       ProtocolDiscovery observatoryDiscovery;
       try {
         observatoryDiscovery = new ProtocolDiscovery.observatory(
-            device.getLogReader(), portForwarder: device.portForwarder);
-        printStatus('Listening.');
+          device.getLogReader(),
+          portForwarder: device.portForwarder,
+        );
+        printStatus('Waiting for a connection from Flutter on ${device.name}...');
         observatoryUri = await observatoryDiscovery.uri;
+        printStatus('Done.');
       } finally {
         await observatoryDiscovery?.cancel();
       }
@@ -90,17 +113,33 @@ class AttachCommand extends FlutterCommand {
       observatoryUri = Uri.parse('http://$ipv4Loopback:$localPort/');
     }
     try {
-      final FlutterDevice flutterDevice =
-          new FlutterDevice(device, trackWidgetCreation: false, previewDart2: argResults['preview-dart-2']);
+      final FlutterDevice flutterDevice = new FlutterDevice(device,
+          trackWidgetCreation: false, previewDart2: argResults['preview-dart-2']);
       flutterDevice.observatoryUris = <Uri>[ observatoryUri ];
       final HotRunner hotRunner = new HotRunner(
         <FlutterDevice>[flutterDevice],
         debuggingOptions: new DebuggingOptions.enabled(getBuildInfo()),
         packagesFilePath: globalResults['packages'],
+        usesTerminalUI: daemon == null,
       );
-      await hotRunner.attach();
+
+      if (daemon != null) {
+        AppInstance app;
+        try {
+          app = await daemon.appDomain.launch(hotRunner, hotRunner.attach,
+              device, null, true, fs.currentDirectory);
+        } catch (error) {
+          throwToolExit(error.toString());
+        }
+        final int result = await app.runner.waitForAppToFinish();
+        if (result != 0)
+          throwToolExit(null, exitCode: result);
+      } else {
+        await hotRunner.attach();
+      }
     } finally {
-      device.portForwarder.forwardedPorts.forEach(device.portForwarder.unforward);
+      final List<ForwardedPort> ports = device.portForwarder.forwardedPorts.toList();
+      ports.forEach(device.portForwarder.unforward);
     }
   }
 

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

@@ -28,7 +28,7 @@ import '../runner/flutter_command.dart';
 import '../tester/flutter_tester.dart';
 import '../vmservice.dart';
 
-const String protocolVersion = '0.4.0';
+const String protocolVersion = '0.4.1';
 
 /// A server process command. This command will start up a long-lived server.
 /// It reads JSON-RPC based commands from stdin, executes them, and returns
@@ -303,6 +303,11 @@ class DaemonDomain extends Domain {
   }
 }
 
+typedef Future<void> _RunOrAttach({
+  Completer<DebugConnectionInfo> connectionInfoCompleter,
+  Completer<void> appStartedCompleter
+});
+
 /// This domain responds to methods like [start] and [stop].
 ///
 /// It fires events for application start, stop, and stdout and stderr.
@@ -369,8 +374,29 @@ class AppDomain extends Domain {
         ipv6: ipv6,
       );
     }
-
-    final AppInstance app = new AppInstance(_getNewAppId(), runner: runner, logToStdout: daemon.logToStdout);
+    
+    return launch(
+        runner,
+        ({ Completer<DebugConnectionInfo> connectionInfoCompleter,
+            Completer<void> appStartedCompleter }) => runner.run(
+                connectionInfoCompleter: connectionInfoCompleter,
+                appStartedCompleter: appStartedCompleter,
+                route: route),
+        device,
+        projectDirectory,
+        enableHotReload,
+        cwd);
+  }
+
+  Future<AppInstance> launch(
+      ResidentRunner runner,
+      _RunOrAttach runOrAttach,
+      Device device,
+      String projectDirectory,
+      bool enableHotReload,
+      Directory cwd) async {
+    final AppInstance app = new AppInstance(_getNewAppId(),
+        runner: runner, logToStdout: daemon.logToStdout);
     _apps.add(app);
     _sendAppEvent(app, 'start', <String, dynamic>{
       'deviceId': device.id,
@@ -380,7 +406,7 @@ class AppDomain extends Domain {
 
     Completer<DebugConnectionInfo> connectionInfoCompleter;
 
-    if (options.debuggingEnabled) {
+    if (runner.debuggingOptions.debuggingEnabled) {
       connectionInfoCompleter = new Completer<DebugConnectionInfo>();
       // We don't want to wait for this future to complete and callbacks won't fail.
       // As it just writes to stdout.
@@ -394,20 +420,18 @@ class AppDomain extends Domain {
         _sendAppEvent(app, 'debugPort', params);
       });
     }
-    final Completer<Null> appStartedCompleter = new Completer<Null>();
+    final Completer<void> appStartedCompleter = new Completer<void>();
     // We don't want to wait for this future to complete and callbacks won't fail.
     // As it just writes to stdout.
-    appStartedCompleter.future.then<Null>((Null value) { // ignore: unawaited_futures
+    appStartedCompleter.future.then<void>((_) { // ignore: unawaited_futures
       _sendAppEvent(app, 'started');
     });
 
     await app._runInZone<Null>(this, () async {
       try {
-        await runner.run(
-          connectionInfoCompleter: connectionInfoCompleter,
-          appStartedCompleter: appStartedCompleter,
-          route: route,
-        );
+        await runOrAttach(
+            connectionInfoCompleter: connectionInfoCompleter,
+            appStartedCompleter: appStartedCompleter);
         _sendAppEvent(app, 'stop');
       } catch (error, trace) {
         _sendAppEvent(app, 'stop', <String, dynamic>{
@@ -419,7 +443,6 @@ class AppDomain extends Domain {
         _apps.remove(app);
       }
     });
-
     return app;
   }
 

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

@@ -391,7 +391,7 @@ class RunCommand extends RunCommandBase {
     // need to know about analytics.
     //
     // Do not add more operations to the future.
-    final Completer<Null> appStartedTimeRecorder = new Completer<Null>.sync();
+    final Completer<void> appStartedTimeRecorder = new Completer<void>.sync();
     // This callback can't throw.
     appStartedTimeRecorder.future.then( // ignore: unawaited_futures
       (_) { appStartedTime = clock.now(); }

packages/flutter_tools/lib/src/resident_runner.dart

@@ -467,7 +467,7 @@ abstract class ResidentRunner {
   /// Start the app and keep the process running during its lifetime.
   Future<int> run({
     Completer<DebugConnectionInfo> connectionInfoCompleter,
-    Completer<Null> appStartedCompleter,
+    Completer<void> appStartedCompleter,
     String route,
     bool shouldBuild = true
   });

packages/flutter_tools/lib/src/run_cold.dart

@@ -35,7 +35,7 @@ class ColdRunner extends ResidentRunner {
   @override
   Future<int> run({
     Completer<DebugConnectionInfo> connectionInfoCompleter,
-    Completer<Null> appStartedCompleter,
+    Completer<void> appStartedCompleter,
     String route,
     bool shouldBuild = true
   }) async {

packages/flutter_tools/lib/src/run_hot.dart

@@ -149,7 +149,7 @@ class HotRunner extends ResidentRunner {
 
   Future<int> attach({
     Completer<DebugConnectionInfo> connectionInfoCompleter,
-    Completer<Null> appStartedCompleter,
+    Completer<void> appStartedCompleter,
     String viewFilter,
   }) async {
     try {
@@ -235,7 +235,7 @@ class HotRunner extends ResidentRunner {
   @override
   Future<int> run({
     Completer<DebugConnectionInfo> connectionInfoCompleter,
-    Completer<Null> appStartedCompleter,
+    Completer<void> appStartedCompleter,
     String route,
     bool shouldBuild = true
   }) async {

packages/flutter_tools/lib/src/tester/flutter_tester.dart

@@ -44,6 +44,7 @@ class FlutterTesterDevice extends Device {
   FlutterTesterDevice(String deviceId) : super(deviceId);
 
   Process _process;
+  final DevicePortForwarder _portForwarder = new _NoopPortForwarder();
 
   @override
   Future<bool> get isLocalEmulator async => false;
@@ -52,7 +53,7 @@ class FlutterTesterDevice extends Device {
   String get name => 'Flutter test device';
 
   @override
-  DevicePortForwarder get portForwarder => null;
+  DevicePortForwarder get portForwarder => _portForwarder;
 
   @override
   Future<String> get sdkNameAndVersion async {
@@ -233,3 +234,20 @@ class _FlutterTesterDeviceLogReader extends DeviceLogReader {
 
   void addLine(String line) => _logLinesController.add(line);
 }
+
+/// A fake port forwarder that doesn't do anything. Used by flutter tester
+/// where the VM is running on the same machine and does not need ports forwarding.
+class _NoopPortForwarder extends DevicePortForwarder {
+  @override
+  Future<int> forward(int devicePort, {int hostPort}) {
+    if (hostPort != null && hostPort != devicePort)
+      throw 'Forwarding to a different port is not supported by flutter tester';
+    return new Future<int>.value(devicePort);
+  }
+
+  @override
+  List<ForwardedPort> get forwardedPorts => <ForwardedPort>[];
+
+  @override
+  Future<Null> unforward(ForwardedPort forwardedPort) => null;
+}

packages/flutter_tools/test/integration/flutter_attach.dart

+// Copyright 2018 The Chromium 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 'package:file/file.dart';
+import 'package:flutter_tools/src/base/file_system.dart';
+import 'package:test/test.dart';
+
+import '../src/context.dart';
+import 'test_data/basic_project.dart';
+import 'test_driver.dart';
+
+FlutterTestDriver _flutterRun, _flutterAttach;
+BasicProject _project = new BasicProject();
+
+void main() {
+
+  setUp(() async {
+    final Directory tempDir = await fs.systemTempDirectory.createTemp('test_app');
+    await _project.setUpIn(tempDir);
+    _flutterRun = new FlutterTestDriver(tempDir);
+    _flutterAttach = new FlutterTestDriver(tempDir);
+  });
+
+  tearDown(() async {
+    try {
+      await _flutterRun.stop();
+      await _flutterAttach.stop();
+      _project.cleanup();
+    } catch (e) {
+      // Don't fail tests if we failed to clean up temp folder.
+    }
+  });
+
+  group('attached process', () {
+    testUsingContext('can hot reload', () async {
+      await _flutterRun.run(withDebugger: true);
+      await _flutterAttach.attach(_flutterRun.vmServicePort);
+      
+      await _flutterAttach.hotReload();
+    });
+  }, timeout: const Timeout.factor(3));
+}

packages/flutter_tools/test/integration/test_driver.dart

@@ -23,7 +23,7 @@ const Duration appStartTimeout = const Duration(seconds: 60);
 const Duration quitTimeout = const Duration(seconds: 5);
 
 class FlutterTestDriver {
-  Directory _projectFolder;
+  final Directory _projectFolder;
   Process _proc;
   int _procPid;
   final StreamController<String> _stdout = new StreamController<String>.broadcast();
@@ -32,11 +32,14 @@ class FlutterTestDriver {
   final StringBuffer _errorBuffer = new StringBuffer();
   String _lastResponse;
   String _currentRunningAppId;
+  Uri _vmServiceWsUri;
+  int _vmServicePort;
 
   FlutterTestDriver(this._projectFolder);
 
   VMServiceClient vmService;
   String get lastErrorInfo => _errorBuffer.toString();
+  int get vmServicePort => _vmServicePort;
 
   String _debugPrint(String msg) {
     const int maxLength = 500;
@@ -47,13 +50,44 @@ class FlutterTestDriver {
       print(truncatedMsg);
     }
     return msg;
-}
+  }
 
   // TODO(dantup): Is there a better way than spawning a proc? This breaks debugging..
   // However, there's a lot of logic inside RunCommand that wouldn't be good
   // to duplicate here.
   Future<void> run({bool withDebugger = false}) async {
-    _proc = await _runFlutter(_projectFolder);
+    await _setupProcess(<String>[
+        'run',
+        '--machine',
+        '-d',
+        'flutter-tester',
+    ], withDebugger: withDebugger);
+  }
+
+  Future<void> attach(int port, {bool withDebugger = false}) async {
+    await _setupProcess(<String>[
+        'attach',
+        '--machine',
+        '-d',
+        'flutter-tester',
+        '--debug-port',
+        '$port',
+    ], withDebugger: withDebugger);
+  }
+
+  Future<void> _setupProcess(List<String> args, {bool withDebugger = false}) async {
+    final String flutterBin = fs.path.join(getFlutterRoot(), 'bin', 'flutter');
+    _debugPrint('Spawning flutter $args in ${_projectFolder.path}');
+    
+    const ProcessManager _processManager = const LocalProcessManager();
+    _proc = await _processManager.start(
+        <String>[flutterBin]
+            .followedBy(args)
+            .followedBy(withDebugger ? <String>['--start-paused'] : <String>[])
+            .toList(),
+        workingDirectory: _projectFolder.path,
+        environment: <String, String>{'FLUTTER_TEST': 'true'});
+
     _transformToLines(_proc.stdout).listen((String line) => _stdout.add(line));
     _transformToLines(_proc.stderr).listen((String line) => _stderr.add(line));
 
@@ -76,14 +110,13 @@ class FlutterTestDriver {
         timeout: appStartTimeout);
 
     if (withDebugger) {
-      final Future<Map<String, dynamic>> debugPort = _waitFor(event: 'app.debugPort',
+      final Map<String, dynamic> debugPort = await _waitFor(event: 'app.debugPort',
           timeout: appStartTimeout);
-      final String wsUriString = (await debugPort)['params']['wsUri'];
-      // Ensure the app is started before we try to connect to it.
-      await started;
-      final Uri uri = Uri.parse(wsUriString);
+      final String wsUriString = debugPort['params']['wsUri'];
+      _vmServiceWsUri = Uri.parse(wsUriString);
+      _vmServicePort = debugPort['params']['port'];
       // Proxy the stream/sink for the VM Client so we can debugPrint it.
-      final StreamChannel<String> channel = new IOWebSocketChannel.connect(uri)
+      final StreamChannel<String> channel = new IOWebSocketChannel.connect(_vmServiceWsUri)
           .cast<String>()
           .changeStream((Stream<String> stream) => stream.map(_debugPrint))
           .changeSink((StreamSink<String> sink) =>
@@ -155,26 +188,6 @@ class FlutterTestDriver {
     return _proc.exitCode;
   }
 
-  Future<Process> _runFlutter(Directory projectDir) async {
-    final String flutterBin = fs.path.join(getFlutterRoot(), 'bin', 'flutter');
-    final List<String> command = <String>[
-        flutterBin,
-        'run',
-        '--machine',
-        '-d',
-        'flutter-tester',
-        '--start-paused',
-    ];
-    _debugPrint('Spawning $command in ${projectDir.path}');
-    
-    const ProcessManager _processManager = const LocalProcessManager();
-    return _processManager.start(
-        command,
-        workingDirectory: projectDir.path,
-        environment: <String, String>{'FLUTTER_TEST': 'true'}
-    );
-  }
-
   Future<void> addBreakpoint(String path, int line) async {
     final VM vm = await vmService.getVM();
     final VMIsolate isolate = await vm.isolates.first.load();

packages/flutter_tools/test/resident_runner_test.dart

@@ -36,7 +36,7 @@ class TestRunner extends ResidentRunner {
   @override
   Future<int> run({
     Completer<DebugConnectionInfo> connectionInfoCompleter,
-    Completer<dynamic> appStartedCompleter,
+    Completer<void> appStartedCompleter,
     String route,
     bool shouldBuild = true,
   }) => null;

packages/flutter_tools/test/tester/flutter_tester_test.dart

@@ -79,7 +79,7 @@ void main() {
       expect(device.id, 'flutter-tester');
       expect(await device.isLocalEmulator, isFalse);
       expect(device.name, 'Flutter test device');
-      expect(device.portForwarder, isNull);
+      expect(device.portForwarder, isNot(isNull));
       expect(await device.targetPlatform, TargetPlatform.tester);
 
       expect(await device.installApp(null), isTrue);