[flutter_tools] allow adb to fail to unforward without crashing (#57173)

packages/flutter_tools/lib/src/android/android_device.dart

@@ -214,7 +214,7 @@ class AndroidDevice extends Device {
 
   AdbLogReader _logReader;
   AdbLogReader _pastLogReader;
-  _AndroidDevicePortForwarder _portForwarder;
+  AndroidDevicePortForwarder _portForwarder;
 
   List<String> adbCommandForDevice(List<String> args) {
     return <String>[getAdbPath(globals.androidSdk), '-s', id, ...args];
@@ -697,7 +697,12 @@ class AndroidDevice extends Device {
   }
 
   @override
-  DevicePortForwarder get portForwarder => _portForwarder ??= _AndroidDevicePortForwarder(this);
+  DevicePortForwarder get portForwarder => _portForwarder ??= AndroidDevicePortForwarder(
+    processManager: globals.processManager,
+    logger: globals.logger,
+    deviceId: id,
+    adbPath: globals.androidSdk.adbPath,
+  );
 
   static final RegExp _timeRegExp = RegExp(r'^\d{2}-\d{2} \d{2}:\d{2}:\d{2}\.\d{3}', multiLine: true);
 
@@ -1088,10 +1093,22 @@ class AdbLogReader extends DeviceLogReader {
   }
 }
 
-class _AndroidDevicePortForwarder extends DevicePortForwarder {
-  _AndroidDevicePortForwarder(this.device);
-
-  final AndroidDevice device;
+/// A [DevicePortForwarder] implemented for Android devices that uses adb.
+class AndroidDevicePortForwarder extends DevicePortForwarder {
+  AndroidDevicePortForwarder({
+    @required ProcessManager processManager,
+    @required Logger logger,
+    @required String deviceId,
+    @required String adbPath,
+  }) : _deviceId = deviceId,
+       _adbPath = adbPath,
+       _logger = logger,
+       _processUtils = ProcessUtils(logger: logger, processManager: processManager);
+
+  final String _deviceId;
+  final String _adbPath;
+  final Logger _logger;
+  final ProcessUtils _processUtils;
 
   static int _extractPort(String portString) {
     return int.tryParse(portString.trim());
@@ -1103,18 +1120,24 @@ class _AndroidDevicePortForwarder extends DevicePortForwarder {
 
     String stdout;
     try {
-      stdout = processUtils.runSync(
-        device.adbCommandForDevice(<String>['forward', '--list']),
+      stdout = _processUtils.runSync(
+        <String>[
+          _adbPath,
+          '-s',
+          _deviceId,
+          'forward',
+          '--list',
+        ],
         throwOnError: true,
       ).stdout.trim();
     } on ProcessException catch (error) {
-      globals.printError('Failed to list forwarded ports: $error.');
+      _logger.printError('Failed to list forwarded ports: $error.');
       return ports;
     }
 
     final List<String> lines = LineSplitter.split(stdout).toList();
     for (final String line in lines) {
-      if (!line.startsWith(device.id)) {
+      if (!line.startsWith(_deviceId)) {
         continue;
       }
       final List<String> splitLine = line.split('tcp:');
@@ -1142,13 +1165,15 @@ class _AndroidDevicePortForwarder extends DevicePortForwarder {
   @override
   Future<int> forward(int devicePort, { int hostPort }) async {
     hostPort ??= 0;
-    final List<String> forwardCommand = <String>[
-      'forward',
-      'tcp:$hostPort',
-      'tcp:$devicePort',
-    ];
-    final RunResult process = await processUtils.run(
-      device.adbCommandForDevice(forwardCommand),
+    final RunResult process = await _processUtils.run(
+      <String>[
+        _adbPath,
+        '-s',
+        _deviceId,
+        'forward',
+        'tcp:$hostPort',
+        'tcp:$devicePort',
+      ],
       throwOnError: true,
     );
 
@@ -1195,15 +1220,25 @@ class _AndroidDevicePortForwarder extends DevicePortForwarder {
 
   @override
   Future<void> unforward(ForwardedPort forwardedPort) async {
-    final List<String> unforwardCommand = <String>[
-      'forward',
-      '--remove',
-      'tcp:${forwardedPort.hostPort}',
-    ];
-    await processUtils.run(
-      device.adbCommandForDevice(unforwardCommand),
-      throwOnError: true,
+    final String tcpLine = 'tcp:${forwardedPort.hostPort}';
+    final RunResult runResult = await _processUtils.run(
+      <String>[
+        _adbPath,
+        '-s',
+        _deviceId,
+        'forward',
+        '--remove',
+        tcpLine,
+      ],
+      throwOnError: false,
     );
+    // The port may have already been unforwarded, for example if there
+    // are multiple attach process already connected.
+    if (runResult.exitCode == 0 || runResult
+      .stderr.contains("listener '$tcpLine' not found")) {
+      return;
+    }
+    runResult.throwException('Process exited abnormally:\n$runResult');
   }
 
   @override

packages/flutter_tools/test/general.shard/android/android_device_port_forwarder_test.dart

+// Copyright 2014 The Flutter 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:flutter_tools/src/android/android_device.dart';
+import 'package:flutter_tools/src/base/io.dart';
+import 'package:flutter_tools/src/base/logger.dart';
+import 'package:flutter_tools/src/device.dart';
+
+import '../../src/common.dart';
+import '../../src/context.dart';
+
+void main() {
+  testWithoutContext('AndroidDevicePortForwarder returns the generated host '
+    'port from stdout', () async {
+    final AndroidDevicePortForwarder forwarder = AndroidDevicePortForwarder(
+      adbPath: 'adb',
+      deviceId: '1',
+      processManager: FakeProcessManager.list(<FakeCommand>[
+        const FakeCommand(
+          command: <String>['adb', '-s', '1', 'forward', 'tcp:0', 'tcp:123'],
+          stdout: '456',
+        )
+      ]),
+      logger: BufferLogger.test(),
+    );
+
+    expect(await forwarder.forward(123), 456);
+  });
+
+  testWithoutContext('AndroidDevicePortForwarder returns the supplied host '
+    'port when stdout is empty', () async {
+    final AndroidDevicePortForwarder forwarder = AndroidDevicePortForwarder(
+      adbPath: 'adb',
+      deviceId: '1',
+      processManager: FakeProcessManager.list(<FakeCommand>[
+        const FakeCommand(
+          command: <String>['adb', '-s', '1', 'forward', 'tcp:456', 'tcp:123'],
+          stdout: '',
+        )
+      ]),
+      logger: BufferLogger.test(),
+    );
+
+    expect(await forwarder.forward(123, hostPort: 456), 456);
+  });
+
+  testWithoutContext('AndroidDevicePortForwarder returns the supplied host port '
+    'when stdout is the host port', () async {
+    final AndroidDevicePortForwarder forwarder = AndroidDevicePortForwarder(
+      adbPath: 'adb',
+      deviceId: '1',
+      processManager: FakeProcessManager.list(<FakeCommand>[
+        const FakeCommand(
+          command: <String>['adb', '-s', '1', 'forward', 'tcp:456', 'tcp:123'],
+          stdout: '456',
+        )
+      ]),
+      logger: BufferLogger.test(),
+    );
+
+    expect(await forwarder.forward(123, hostPort: 456), 456);
+  });
+
+  testWithoutContext('AndroidDevicePortForwarder throws an exception when stdout '
+    'is not blank nor the host port', () async {
+    final AndroidDevicePortForwarder forwarder = AndroidDevicePortForwarder(
+      adbPath: 'adb',
+      deviceId: '1',
+      processManager: FakeProcessManager.list(<FakeCommand>[
+        const FakeCommand(
+          command: <String>['adb', '-s', '1', 'forward', 'tcp:456', 'tcp:123'],
+          stdout: '123456',
+        )
+      ]),
+      logger: BufferLogger.test(),
+    );
+
+    expect(forwarder.forward(123, hostPort: 456), throwsA(isA<ProcessException>()));
+  });
+
+  testWithoutContext('AndroidDevicePortForwarder forwardedPorts returns empty '
+    'list when forward failed', () {
+    final AndroidDevicePortForwarder forwarder = AndroidDevicePortForwarder(
+      adbPath: 'adb',
+      deviceId: '1',
+      processManager: FakeProcessManager.list(<FakeCommand>[
+        const FakeCommand(
+          command: <String>['adb', '-s', '1', 'forward', '--list'],
+          exitCode: 1,
+        )
+      ]),
+      logger: BufferLogger.test(),
+    );
+
+    expect(forwarder.forwardedPorts, isEmpty);
+  });
+
+  testWithoutContext('disposing device disposes the portForwarder', () async {
+    final FakeProcessManager processManager = FakeProcessManager.list(<FakeCommand>[
+      const FakeCommand(
+        command: <String>['adb', '-s', '1', 'forward', 'tcp:0', 'tcp:123'],
+        stdout: '456',
+      ),
+      const FakeCommand(
+        command: <String>['adb', '-s', '1', 'forward', '--list'],
+        stdout: '1234 tcp:456 tcp:123',
+      ),
+      const FakeCommand(
+        command: <String>['adb', '-s', '1', 'forward', '--remove', 'tcp:456'],
+      )
+    ]);
+    final AndroidDevicePortForwarder forwarder = AndroidDevicePortForwarder(
+      adbPath: 'adb',
+      deviceId: '1',
+      processManager: processManager,
+      logger: BufferLogger.test(),
+    );
+
+    expect(await forwarder.forward(123), equals(456));
+
+    await forwarder.dispose();
+
+    expect(processManager.hasRemainingExpectations, false);
+  });
+
+  testWithoutContext('failures to unforward port do not throw if the forward is missing', () async {
+    final FakeProcessManager processManager = FakeProcessManager.list(<FakeCommand>[
+      const FakeCommand(
+        command: <String>['adb', '-s', '1', 'forward', '--remove', 'tcp:456'],
+        stderr: "error: listener 'tcp:456' not found",
+        exitCode: 1,
+      )
+    ]);
+    final AndroidDevicePortForwarder forwarder = AndroidDevicePortForwarder(
+      adbPath: 'adb',
+      deviceId: '1',
+      processManager: processManager,
+      logger: BufferLogger.test(),
+    );
+
+    await forwarder.unforward(ForwardedPort(456, 23));
+  });
+
+  testWithoutContext('failures to unforward port throw exception if stderr is not recognized', () async {
+    final FakeProcessManager processManager = FakeProcessManager.list(<FakeCommand>[
+      const FakeCommand(
+        command: <String>['adb', '-s', '1', 'forward', '--remove', 'tcp:456'],
+        stderr: 'error: everything is broken!',
+        exitCode: 1,
+      )
+    ]);
+    final AndroidDevicePortForwarder forwarder = AndroidDevicePortForwarder(
+      adbPath: 'adb',
+      deviceId: '1',
+      processManager: processManager,
+      logger: BufferLogger.test(),
+    );
+
+    expect(() => forwarder.unforward(ForwardedPort(456, 23)), throwsA(isA<ProcessException>()));
+  });
+}

packages/flutter_tools/test/general.shard/android/android_device_test.dart

@@ -387,87 +387,6 @@ flutter:
     });
   });
 
-  group('portForwarder', () {
-    final ProcessManager mockProcessManager = MockProcessManager();
-    final AndroidDevice device = AndroidDevice('1234');
-    final DevicePortForwarder forwarder = device.portForwarder;
-
-    testUsingContext('returns the generated host port from stdout', () async {
-      when(mockProcessManager.run(argThat(contains('forward'))))
-          .thenAnswer((_) async => ProcessResult(0, 0, '456', ''));
-
-      expect(await forwarder.forward(123), equals(456));
-    }, overrides: <Type, Generator>{
-      ProcessManager: () => mockProcessManager,
-    });
-
-    testUsingContext('returns the supplied host port when stdout is empty', () async {
-      when(mockProcessManager.run(argThat(contains('forward'))))
-          .thenAnswer((_) async => ProcessResult(0, 0, '', ''));
-
-      expect(await forwarder.forward(123, hostPort: 456), equals(456));
-    }, overrides: <Type, Generator>{
-      ProcessManager: () => mockProcessManager,
-    });
-
-    testUsingContext('returns the supplied host port when stdout is the host port', () async {
-      when(mockProcessManager.run(argThat(contains('forward'))))
-          .thenAnswer((_) async => ProcessResult(0, 0, '456', ''));
-
-      expect(await forwarder.forward(123, hostPort: 456), equals(456));
-    }, overrides: <Type, Generator>{
-      ProcessManager: () => mockProcessManager,
-    });
-
-    testUsingContext('throws an error when stdout is not blank nor the host port', () async {
-      when(mockProcessManager.run(argThat(contains('forward'))))
-          .thenAnswer((_) async => ProcessResult(0, 0, '123456', ''));
-
-      expect(forwarder.forward(123, hostPort: 456), throwsA(isA<ProcessException>()));
-    }, overrides: <Type, Generator>{
-      ProcessManager: () => mockProcessManager,
-    });
-
-    testUsingContext('forwardedPorts returns empty list when forward failed', () {
-      when(mockProcessManager.runSync(argThat(contains('forward'))))
-          .thenReturn(ProcessResult(0, 1, '', ''));
-
-      expect(forwarder.forwardedPorts, equals(const <ForwardedPort>[]));
-    }, overrides: <Type, Generator>{
-      ProcessManager: () => mockProcessManager,
-    });
-
-    testUsingContext('disposing device disposes the portForwarder', () async {
-      bool unforwardCalled = false;
-      when(mockProcessManager.run(argThat(containsAll(<String>[
-        'forward',
-        'tcp:0',
-        'tcp:123',
-      ])))).thenAnswer((_) async {
-        return ProcessResult(0, 0, '456', '');
-      });
-      when(mockProcessManager.runSync(argThat(containsAll(<String>[
-        'forward',
-        '--list',
-      ])))).thenReturn(ProcessResult(0, 0, '1234 tcp:456 tcp:123', ''));
-      when(mockProcessManager.run(argThat(containsAll(<String>[
-        'forward',
-        '--remove',
-        'tcp:456',
-      ])))).thenAnswer((_) async {
-        unforwardCalled = true;
-        return ProcessResult(0, 0, '', '');
-      });
-      expect(await forwarder.forward(123), equals(456));
-
-      await device.dispose();
-
-      expect(unforwardCalled, isTrue);
-    }, overrides: <Type, Generator>{
-      ProcessManager: () => mockProcessManager,
-    });
-  });
-
   group('logcat', () {
     final ProcessManager mockProcessManager = MockProcessManager();
     final AndroidDevice device = AndroidDevice('1234');