Stop leaking iproxy processes (#42026)

packages/flutter_tools/lib/src/device.dart

@@ -12,6 +12,7 @@ import 'application_package.dart';
 import 'artifacts.dart';
 import 'base/context.dart';
 import 'base/file_system.dart';
+import 'base/io.dart';
 import 'base/utils.dart';
 import 'build_info.dart';
 import 'fuchsia/fuchsia_device.dart';
@@ -473,6 +474,11 @@ abstract class Device {
   static Future<void> printDevices(List<Device> devices) async {
     await descriptions(devices).forEach(printStatus);
   }
+
+  /// Clean up resources allocated by device
+  ///
+  /// For example log readers or port forwarders.
+  void dispose() {}
 }
 
 class DebuggingOptions {
@@ -564,6 +570,15 @@ class ForwardedPort {
 
   @override
   String toString() => 'ForwardedPort HOST:$hostPort to DEVICE:$devicePort';
+
+  /// Kill subprocess (if present) used in forwarding.
+  void dispose() {
+    final Process process = context;
+
+    if (process != null) {
+      process.kill();
+    }
+  }
 }
 
 /// Forward ports from the host machine to the device.
@@ -579,6 +594,9 @@ abstract class DevicePortForwarder {
 
   /// Stops forwarding [forwardedPort].
   Future<void> unforward(ForwardedPort forwardedPort);
+
+  /// Cleanup allocated resources, like forwardedPorts
+  Future<void> dispose() async { }
 }
 
 /// Read the log for a particular device.
@@ -593,6 +611,9 @@ abstract class DeviceLogReader {
 
   /// Process ID of the app on the device.
   int appPid;
+
+  // Clean up resources allocated by log reader e.g. subprocesses
+  void dispose() { }
 }
 
 /// Describes an app running on the device.
@@ -614,6 +635,9 @@ class NoOpDeviceLogReader implements DeviceLogReader {
 
   @override
   Stream<String> get logLines => const Stream<String>.empty();
+
+  @override
+  void dispose() { }
 }
 
 // A portforwarder which does not support forwarding ports.
@@ -628,4 +652,7 @@ class NoOpDevicePortForwarder implements DevicePortForwarder {
 
   @override
   Future<void> unforward(ForwardedPort forwardedPort) async { }
+
+  @override
+  Future<void> dispose() async { }
 }

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

@@ -14,7 +14,6 @@ import '../base/io.dart';
 import '../base/logger.dart';
 import '../base/platform.dart';
 import '../base/process.dart';
-import '../base/process_manager.dart';
 import '../build_info.dart';
 import '../convert.dart';
 import '../device.dart';
@@ -417,7 +416,7 @@ class IOSDevice extends Device {
   @override
   DeviceLogReader getLogReader({ ApplicationPackage app }) {
     _logReaders ??= <ApplicationPackage, DeviceLogReader>{};
-    return _logReaders.putIfAbsent(app, () => _IOSDeviceLogReader(this, app));
+    return _logReaders.putIfAbsent(app, () => IOSDeviceLogReader(this, app));
   }
 
   @visibleForTesting
@@ -427,7 +426,7 @@ class IOSDevice extends Device {
   }
 
   @override
-  DevicePortForwarder get portForwarder => _portForwarder ??= _IOSDevicePortForwarder(this);
+  DevicePortForwarder get portForwarder => _portForwarder ??= IOSDevicePortForwarder(this);
 
   @visibleForTesting
   set portForwarder(DevicePortForwarder forwarder) {
@@ -449,6 +448,14 @@ class IOSDevice extends Device {
   bool isSupportedForProject(FlutterProject flutterProject) {
     return flutterProject.ios.existsSync();
   }
+
+  @override
+  void dispose() {
+    _logReaders.forEach((ApplicationPackage application, DeviceLogReader logReader) {
+      logReader.dispose();
+    });
+    _portForwarder?.dispose();
+  }
 }
 
 /// Decodes a vis-encoded syslog string to a UTF-8 representation.
@@ -511,11 +518,12 @@ String decodeSyslog(String line) {
   }
 }
 
-class _IOSDeviceLogReader extends DeviceLogReader {
-  _IOSDeviceLogReader(this.device, ApplicationPackage app) {
+@visibleForTesting
+class IOSDeviceLogReader extends DeviceLogReader {
+  IOSDeviceLogReader(this.device, ApplicationPackage app) {
     _linesController = StreamController<String>.broadcast(
       onListen: _start,
-      onCancel: _stop,
+      onCancel: dispose,
     );
 
     // Match for lines for the runner in syslog.
@@ -538,7 +546,6 @@ class _IOSDeviceLogReader extends DeviceLogReader {
   RegExp _anyLineRegex;
 
   StreamController<String> _linesController;
-  Process _process;
 
   @override
   Stream<String> get logLines => _linesController.stream;
@@ -548,17 +555,22 @@ class _IOSDeviceLogReader extends DeviceLogReader {
 
   void _start() {
     iMobileDevice.startLogger(device.id).then<void>((Process process) {
-      _process = process;
-      _process.stdout.transform<String>(utf8.decoder).transform<String>(const LineSplitter()).listen(_newLineHandler());
-      _process.stderr.transform<String>(utf8.decoder).transform<String>(const LineSplitter()).listen(_newLineHandler());
-      _process.exitCode.whenComplete(() {
+      process.stdout.transform<String>(utf8.decoder).transform<String>(const LineSplitter()).listen(_newLineHandler());
+      process.stderr.transform<String>(utf8.decoder).transform<String>(const LineSplitter()).listen(_newLineHandler());
+      process.exitCode.whenComplete(() {
         if (_linesController.hasListener) {
           _linesController.close();
         }
       });
+      assert(_idevicesyslogProcess == null);
+      _idevicesyslogProcess = process;
     });
   }
 
+  @visibleForTesting
+  set idevicesyslogProcess(Process process) => _idevicesyslogProcess = process;
+  Process _idevicesyslogProcess;
+
   // Returns a stateful line handler to properly capture multi-line output.
   //
   // For multi-line log messages, any line after the first is logged without
@@ -590,13 +602,15 @@ class _IOSDeviceLogReader extends DeviceLogReader {
     };
   }
 
-  void _stop() {
-    _process?.kill();
+  @override
+  void dispose() {
+    _idevicesyslogProcess?.kill();
   }
 }
 
-class _IOSDevicePortForwarder extends DevicePortForwarder {
-  _IOSDevicePortForwarder(this.device) : _forwardedPorts = <ForwardedPort>[];
+@visibleForTesting
+class IOSDevicePortForwarder extends DevicePortForwarder {
+  IOSDevicePortForwarder(this.device) : _forwardedPorts = <ForwardedPort>[];
 
   final IOSDevice device;
 
@@ -605,6 +619,11 @@ class _IOSDevicePortForwarder extends DevicePortForwarder {
   @override
   List<ForwardedPort> get forwardedPorts => _forwardedPorts;
 
+  @visibleForTesting
+  void addForwardedPorts(List<ForwardedPort> forwardedPorts) {
+    forwardedPorts.forEach(_forwardedPorts.add);
+  }
+
   static const Duration _kiProxyPortForwardTimeout = Duration(seconds: 1);
 
   @override
@@ -618,7 +637,7 @@ class _IOSDevicePortForwarder extends DevicePortForwarder {
 
     bool connected = false;
     while (!connected) {
-      printTrace('attempting to forward device port $devicePort to host port $hostPort');
+      printTrace('Attempting to forward device port $devicePort to host port $hostPort');
       // Usage: iproxy LOCAL_TCP_PORT DEVICE_TCP_PORT UDID
       process = await processUtils.start(
         <String>[
@@ -634,6 +653,7 @@ class _IOSDevicePortForwarder extends DevicePortForwarder {
       // TODO(ianh): This is a flakey race condition, https://github.com/libimobiledevice/libimobiledevice/issues/674
       connected = !await process.stdout.isEmpty.timeout(_kiProxyPortForwardTimeout, onTimeout: () => false);
       if (!connected) {
+        process.kill();
         if (autoselect) {
           hostPort += 1;
           if (hostPort > 65535) {
@@ -663,13 +683,13 @@ class _IOSDevicePortForwarder extends DevicePortForwarder {
     }
 
     printTrace('Unforwarding port $forwardedPort');
+    forwardedPort.dispose();
+  }
 
-    final Process process = forwardedPort.context;
-
-    if (process != null) {
-      processManager.killPid(process.pid);
-    } else {
-      printError('Forwarded port did not have a valid process');
+  @override
+  Future<void> dispose() async {
+    for (ForwardedPort forwardedPort in _forwardedPorts) {
+      forwardedPort.dispose();
     }
   }
 }

packages/flutter_tools/lib/src/mdns_discovery.dart

@@ -60,6 +60,7 @@ class MDnsObservatoryDiscovery {
           )
           .toList();
       if (pointerRecords.isEmpty) {
+        printTrace('No pointer records found.');
         return null;
       }
       // We have no guarantee that we won't get multiple hits from the same

packages/flutter_tools/lib/src/protocol_discovery.dart

@@ -48,7 +48,13 @@ class ProtocolDiscovery {
   StreamSubscription<String> _deviceLogSubscription;
 
   /// The discovered service URI.
-  Future<Uri> get uri => _completer.future;
+  ///
+  /// Port forwarding is only attempted when this is invoked, in case we never
+  /// need to port forward.
+  Future<Uri> get uri async {
+    final Uri rawUri = await _completer.future;
+    return await _forwardPort(rawUri);
+  }
 
   Future<void> cancel() => _stopScrapingLogs();
 
@@ -74,9 +80,8 @@ class ProtocolDiscovery {
     if (uri != null) {
       assert(!_completer.isCompleted);
       _stopScrapingLogs();
-      _completer.complete(_forwardPort(uri));
+      _completer.complete(uri);
     }
-
   }
 
   Future<Uri> _forwardPort(Uri deviceUri) async {

packages/flutter_tools/lib/src/run_hot.dart

@@ -1031,6 +1031,9 @@ class HotRunner extends ResidentRunner {
 
   @override
   Future<void> cleanupAtFinish() async {
+    for (FlutterDevice flutterDevice in flutterDevices) {
+      flutterDevice.device.dispose();
+    }
     await _cleanupDevFS();
     await stopEchoingDeviceLog();
   }

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

@@ -6,7 +6,9 @@ import 'dart:async';
 
 import 'package:flutter_tools/src/build_info.dart';
 import 'package:flutter_tools/src/device.dart';
+import 'package:flutter_tools/src/base/io.dart';
 import 'package:flutter_tools/src/project.dart';
+import 'package:mockito/mockito.dart';
 
 import '../src/common.dart';
 import '../src/context.dart';
@@ -142,6 +144,23 @@ void main() {
       ]);
     });
   });
+  group('ForwardedPort', () {
+    group('dispose()', () {
+      testUsingContext('does not throw exception if no process is present', () {
+        final ForwardedPort forwardedPort = ForwardedPort(123, 456);
+        expect(forwardedPort.context, isNull);
+        forwardedPort.dispose();
+      });
+
+      testUsingContext('kills process if process was available', () {
+        final MockProcess mockProcess = MockProcess();
+        final ForwardedPort forwardedPort = ForwardedPort.withContext(123, 456, mockProcess);
+        forwardedPort.dispose();
+        expect(forwardedPort.context, isNotNull);
+        verify(mockProcess.kill());
+      });
+    });
+  });
 }
 
 class TestDeviceManager extends DeviceManager {
@@ -186,3 +205,5 @@ class _MockDevice extends Device {
   @override
   bool isSupportedForProject(FlutterProject flutterProject) => _isSupported;
 }
+
+class MockProcess extends Mock implements Process {}

packages/flutter_tools/test/general.shard/ios/devices_test.dart

@@ -35,13 +35,17 @@ import '../../src/context.dart';
 import '../../src/mocks.dart';
 
 class MockIOSApp extends Mock implements IOSApp {}
+class MockApplicationPackage extends Mock implements ApplicationPackage {}
 class MockArtifacts extends Mock implements Artifacts {}
 class MockCache extends Mock implements Cache {}
 class MockDirectory extends Mock implements Directory {}
 class MockFileSystem extends Mock implements FileSystem {}
+class MockForwardedPort extends Mock implements ForwardedPort {}
 class MockIMobileDevice extends Mock implements IMobileDevice {}
 class MockIOSDeploy extends Mock implements IOSDeploy {}
+class MockDevicePortForwarder extends Mock implements DevicePortForwarder {}
 class MockMDnsObservatoryDiscovery extends Mock implements MDnsObservatoryDiscovery {}
+class MockMDnsObservatoryDiscoveryResult extends Mock implements MDnsObservatoryDiscoveryResult {}
 class MockXcode extends Mock implements Xcode {}
 class MockFile extends Mock implements File {}
 class MockPortForwarder extends Mock implements DevicePortForwarder {}
@@ -75,6 +79,65 @@ void main() {
       });
     }
 
+    group('.dispose()', () {
+      IOSDevice device;
+      MockApplicationPackage appPackage1;
+      MockApplicationPackage appPackage2;
+      IOSDeviceLogReader logReader1;
+      IOSDeviceLogReader logReader2;
+      MockProcess mockProcess1;
+      MockProcess mockProcess2;
+      MockProcess mockProcess3;
+      IOSDevicePortForwarder portForwarder;
+      ForwardedPort forwardedPort;
+
+      IOSDevicePortForwarder createPortForwarder(
+          ForwardedPort forwardedPort,
+          IOSDevice device) {
+        final IOSDevicePortForwarder portForwarder = IOSDevicePortForwarder(device);
+        portForwarder.addForwardedPorts(<ForwardedPort>[forwardedPort]);
+        return portForwarder;
+      }
+
+      IOSDeviceLogReader createLogReader(
+          IOSDevice device,
+          ApplicationPackage appPackage,
+          Process process) {
+        final IOSDeviceLogReader logReader = IOSDeviceLogReader(device, appPackage);
+        logReader.idevicesyslogProcess = process;
+        return logReader;
+      }
+
+      setUp(() {
+        appPackage1 = MockApplicationPackage();
+        appPackage2 = MockApplicationPackage();
+        when(appPackage1.name).thenReturn('flutterApp1');
+        when(appPackage2.name).thenReturn('flutterApp2');
+        mockProcess1 = MockProcess();
+        mockProcess2 = MockProcess();
+        mockProcess3 = MockProcess();
+        forwardedPort = ForwardedPort.withContext(123, 456, mockProcess3);
+      });
+
+      testUsingContext(' kills all log readers & port forwarders', () async {
+        device = IOSDevice('123');
+        logReader1 = createLogReader(device, appPackage1, mockProcess1);
+        logReader2 = createLogReader(device, appPackage2, mockProcess2);
+        portForwarder = createPortForwarder(forwardedPort, device);
+        device.setLogReader(appPackage1, logReader1);
+        device.setLogReader(appPackage2, logReader2);
+        device.portForwarder = portForwarder;
+
+        device.dispose();
+
+        verify(mockProcess1.kill());
+        verify(mockProcess2.kill());
+        verify(mockProcess3.kill());
+      }, overrides: <Type, Generator>{
+        Platform: () => macPlatform,
+      });
+    });
+
     group('startApp', () {
       MockIOSApp mockApp;
       MockArtifacts mockArtifacts;
@@ -95,7 +158,7 @@ void main() {
       const int hostPort = 42;
       const String installerPath = '/path/to/ideviceinstaller';
       const String iosDeployPath = '/path/to/iosdeploy';
-      // const String appId = '789';
+      const String iproxyPath = '/path/to/iproxy';
       const MapEntry<String, String> libraryEntry = MapEntry<String, String>(
           'DYLD_LIBRARY_PATH',
           '/path/to/libraries',
@@ -137,6 +200,13 @@ void main() {
             ),
         ).thenReturn(iosDeployPath);
 
+        when(
+            mockArtifacts.getArtifactPath(
+                Artifact.iproxy,
+                platform: anyNamed('platform'),
+            ),
+        ).thenReturn(iproxyPath);
+
         when(mockPortForwarder.forward(devicePort, hostPort: anyNamed('hostPort')))
           .thenAnswer((_) async => hostPort);
         when(mockPortForwarder.forwardedPorts)
@@ -201,6 +271,45 @@ void main() {
         Usage: () => mockUsage,
       });
 
+      // By default, the .forward() method will try every port between 1024
+      // and 65535; this test verifies we are killing iproxy processes when
+      // we timeout on a port
+      testUsingContext(' .forward() will kill iproxy processes before invoking a second', () async {
+        const String deviceId = '123';
+        const int devicePort = 456;
+        final IOSDevice device = IOSDevice(deviceId);
+        final IOSDevicePortForwarder portForwarder = IOSDevicePortForwarder(device);
+        bool firstRun = true;
+        final MockProcess successProcess = MockProcess(
+          exitCode: Future<int>.value(0),
+          stdout: Stream<List<int>>.fromIterable(<List<int>>['Hello'.codeUnits]),
+        );
+        final MockProcess failProcess = MockProcess(
+          exitCode: Future<int>.value(1),
+          stdout: const Stream<List<int>>.empty(),
+        );
+
+        final ProcessFactory factory = (List<String> command) {
+          if (!firstRun) {
+            return successProcess;
+          }
+          firstRun = false;
+          return failProcess;
+        };
+        mockProcessManager.processFactory = factory;
+        final int hostPort = await portForwarder.forward(devicePort);
+        // First port tried (1024) should fail, then succeed on the next
+        expect(hostPort, 1024 + 1);
+        verifyNever(successProcess.kill());
+        verify(failProcess.kill());
+      }, overrides: <Type, Generator>{
+        Artifacts: () => mockArtifacts,
+        Cache: () => mockCache,
+        Platform: () => macPlatform,
+        ProcessManager: () => mockProcessManager,
+        Usage: () => mockUsage,
+      });
+
       testUsingContext(' succeeds in debug mode when mDNS fails by falling back to manual protocol discovery', () async {
         final IOSDevice device = IOSDevice('123');
         device.portForwarder = mockPortForwarder;

packages/flutter_tools/test/src/mocks.dart

@@ -570,6 +570,7 @@ class MockDeviceLogReader extends DeviceLogReader {
 
   void addLine(String line) => _linesController.add(line);
 
+  @override
   void dispose() {
     _linesController.close();
   }