[flutter_tool] Make Device.dispose() abstract (#46006)

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

@@ -719,6 +719,12 @@ class AndroidDevice extends Device {
   bool isSupportedForProject(FlutterProject flutterProject) {
     return flutterProject.android.existsSync();
   }
+
+  @override
+  Future<void> dispose() async {
+    _logReader?._stop();
+    await _portForwarder?.dispose();
+  }
 }
 
 Map<String, String> parseAdbDeviceProperties(String str) {
@@ -1123,10 +1129,13 @@ class _AdbLogReader extends DeviceLogReader {
   }
 
   void _stop() {
-    // TODO(devoncarew): We should remove adb port forwarding here.
-
     _process?.kill();
   }
+
+  @override
+  void dispose() {
+    _stop();
+  }
 }
 
 class _AndroidDevicePortForwarder extends DevicePortForwarder {
@@ -1135,7 +1144,6 @@ class _AndroidDevicePortForwarder extends DevicePortForwarder {
   final AndroidDevice device;
 
   static int _extractPort(String portString) {
-
     return int.tryParse(portString.trim());
   }
 
@@ -1247,4 +1255,11 @@ class _AndroidDevicePortForwarder extends DevicePortForwarder {
       throwOnError: true,
     );
   }
+
+  @override
+  Future<void> dispose() async {
+    for (ForwardedPort port in forwardedPorts) {
+      await unforward(port);
+    }
+  }
 }

packages/flutter_tools/lib/src/desktop_device.dart

@@ -137,6 +137,11 @@ abstract class DesktopDevice extends Device {
     return succeeded;
   }
 
+  @override
+  Future<void> dispose() async {
+    await portForwarder?.dispose();
+  }
+
   /// Builds the current project for this device, with the given options.
   Future<void> buildForDevice(
     ApplicationPackage package, {
@@ -173,4 +178,9 @@ class DesktopLogReader extends DeviceLogReader {
 
   @override
   String get name => 'desktop';
+
+  @override
+  void dispose() {
+    // Nothing to dispose.
+  }
 }

packages/flutter_tools/lib/src/device.dart

@@ -492,7 +492,7 @@ abstract class Device {
   /// Clean up resources allocated by device
   ///
   /// For example log readers or port forwarders.
-  void dispose() {}
+  Future<void> dispose();
 }
 
 /// Information about an application's memory usage.
@@ -633,7 +633,7 @@ abstract class DevicePortForwarder {
   Future<void> unforward(ForwardedPort forwardedPort);
 
   /// Cleanup allocated resources, like forwardedPorts
-  Future<void> dispose() async { }
+  Future<void> dispose();
 }
 
 /// Read the log for a particular device.
@@ -654,7 +654,7 @@ abstract class DeviceLogReader {
   int appPid;
 
   // Clean up resources allocated by log reader e.g. subprocesses
-  void dispose() { }
+  void dispose();
 }
 
 /// Describes an app running on the device.

packages/flutter_tools/lib/src/fuchsia/fuchsia_device.dart

@@ -93,6 +93,12 @@ class _FuchsiaLogReader extends DeviceLogReader {
 
   @override
   String toString() => name;
+
+  @override
+  void dispose() {
+    // The Fuchsia SDK syslog process is killed when the subscription to the
+    // logLines Stream is canceled.
+  }
 }
 
 class _FuchsiaLogSink implements EventSink<String> {
@@ -458,7 +464,12 @@ class FuchsiaDevice extends Device {
   @override
   DevicePortForwarder get portForwarder =>
       _portForwarder ??= _FuchsiaPortForwarder(this);
-  _FuchsiaPortForwarder _portForwarder;
+  DevicePortForwarder _portForwarder;
+
+  @visibleForTesting
+  set portForwarder(DevicePortForwarder forwarder) {
+    _portForwarder = forwarder;
+  }
 
   @override
   void clearLogs() {}
@@ -583,6 +594,11 @@ class FuchsiaDevice extends Device {
   bool isSupportedForProject(FlutterProject flutterProject) {
     return flutterProject.fuchsia.existsSync();
   }
+
+  @override
+  Future<void> dispose() async {
+    await _portForwarder?.dispose();
+  }
 }
 
 class FuchsiaIsolateDiscoveryProtocol {
@@ -736,4 +752,11 @@ class _FuchsiaPortForwarder extends DevicePortForwarder {
       throwToolExit('Unforward command failed: $result');
     }
   }
+
+  @override
+  Future<void> dispose() async {
+    for (ForwardedPort port in forwardedPorts) {
+      await unforward(port);
+    }
+  }
 }

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

@@ -472,11 +472,11 @@ class IOSDevice extends Device {
   }
 
   @override
-  void dispose() {
+  Future<void> dispose() async {
     _logReaders.forEach((IOSApp application, DeviceLogReader logReader) {
       logReader.dispose();
     });
-    _portForwarder?.dispose();
+    await _portForwarder?.dispose();
   }
 }
 

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

@@ -536,6 +536,16 @@ class IOSSimulator extends Device {
   bool isSupportedForProject(FlutterProject flutterProject) {
     return flutterProject.ios.existsSync();
   }
+
+  @override
+  Future<void> dispose() async {
+    _logReaders?.forEach(
+      (ApplicationPackage application, _IOSSimulatorLogReader logReader) {
+        logReader.dispose();
+      },
+    );
+    await _portForwarder?.dispose();
+  }
 }
 
 /// Launches the device log reader process on the host.
@@ -721,6 +731,11 @@ class _IOSSimulatorLogReader extends DeviceLogReader {
     _deviceProcess?.kill();
     _systemProcess?.kill();
   }
+
+  @override
+  void dispose() {
+    _stop();
+  }
 }
 
 int compareIosVersions(String v1, String v2) {
@@ -777,9 +792,7 @@ class _IOSSimulatorDevicePortForwarder extends DevicePortForwarder {
   final List<ForwardedPort> _ports = <ForwardedPort>[];
 
   @override
-  List<ForwardedPort> get forwardedPorts {
-    return _ports;
-  }
+  List<ForwardedPort> get forwardedPorts => _ports;
 
   @override
   Future<int> forward(int devicePort, { int hostPort }) async {
@@ -795,4 +808,11 @@ class _IOSSimulatorDevicePortForwarder extends DevicePortForwarder {
   Future<void> unforward(ForwardedPort forwardedPort) async {
     _ports.remove(forwardedPort);
   }
+
+  @override
+  Future<void> dispose() async {
+    for (ForwardedPort port in _ports) {
+      await unforward(port);
+    }
+  }
 }

packages/flutter_tools/lib/src/run_cold.dart

@@ -171,7 +171,7 @@ class ColdRunner extends ResidentRunner {
   @override
   Future<void> cleanupAtFinish() async {
     for (FlutterDevice flutterDevice in flutterDevices) {
-      flutterDevice.device.dispose();
+      await flutterDevice.device.dispose();
     }
 
     await stopEchoingDeviceLog();

packages/flutter_tools/lib/src/run_hot.dart

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

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

@@ -212,6 +212,12 @@ class FlutterTesterDevice extends Device {
 
   @override
   bool isSupportedForProject(FlutterProject flutterProject) => true;
+
+  @override
+  Future<void> dispose() async {
+    _logReader?.dispose();
+    await _portForwarder?.dispose();
+  }
 }
 
 class FlutterTesterDevices extends PollingDeviceDiscovery {
@@ -250,6 +256,9 @@ class _FlutterTesterDeviceLogReader extends DeviceLogReader {
   String get name => 'flutter tester log reader';
 
   void addLine(String line) => _logLinesController.add(line);
+
+  @override
+  void dispose() {}
 }
 
 /// A fake port forwarder that doesn't do anything. Used by flutter tester
@@ -268,4 +277,7 @@ class _NoopPortForwarder extends DevicePortForwarder {
 
   @override
   Future<void> unforward(ForwardedPort forwardedPort) async { }
+
+  @override
+  Future<void> dispose() async { }
 }

packages/flutter_tools/lib/src/web/web_device.dart

@@ -59,9 +59,11 @@ class ChromeDevice extends Device {
   @override
   void clearLogs() { }
 
+  DeviceLogReader _logReader;
+
   @override
   DeviceLogReader getLogReader({ApplicationPackage app}) {
-    return NoOpDeviceLogReader(app?.name);
+    return _logReader ??= NoOpDeviceLogReader(app?.name);
   }
 
   @override
@@ -160,6 +162,12 @@ class ChromeDevice extends Device {
   bool isSupportedForProject(FlutterProject flutterProject) {
     return flutterProject.web.existsSync();
   }
+
+  @override
+  Future<void> dispose() async {
+    _logReader?.dispose();
+    await portForwarder?.dispose();
+  }
 }
 
 class WebDevices extends PollingDeviceDiscovery {
@@ -206,9 +214,11 @@ class WebServerDevice extends Device {
   @override
   Future<String> get emulatorId => null;
 
+  DeviceLogReader _logReader;
+
   @override
   DeviceLogReader getLogReader({ApplicationPackage app}) {
-    return NoOpDeviceLogReader(app.name);
+    return _logReader ??= NoOpDeviceLogReader(app.name);
   }
 
   @override
@@ -271,4 +281,10 @@ class WebServerDevice extends Device {
   Future<bool> uninstallApp(ApplicationPackage app) async {
     return true;
   }
+
+  @override
+  Future<void> dispose() async {
+    _logReader?.dispose();
+    await portForwarder?.dispose();
+  }
 }

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

@@ -665,6 +665,36 @@ flutter:
     }, 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('lastLogcatTimestamp', () {

packages/flutter_tools/test/general.shard/fuchsia/fuchsia_device_test.dart

@@ -71,6 +71,14 @@ void main() {
       expect(names.length, 0);
     });
 
+    testUsingContext('disposing device disposes the portForwarder', () async {
+      final MockPortForwarder mockPortForwarder = MockPortForwarder();
+      final FuchsiaDevice device = FuchsiaDevice('123');
+      device.portForwarder = mockPortForwarder;
+      await device.dispose();
+      verify(mockPortForwarder.dispose()).called(1);
+    });
+
     testUsingContext('default capabilities', () async {
       final FuchsiaDevice device = FuchsiaDevice('123');
       fs.directory('fuchsia').createSync(recursive: true);

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

@@ -138,7 +138,7 @@ void main() {
         device.setLogReader(appPackage2, logReader2);
         device.portForwarder = portForwarder;
 
-        device.dispose();
+        await device.dispose();
 
         verify(mockProcess1.kill());
         verify(mockProcess2.kill());
@@ -249,6 +249,16 @@ void main() {
         Cache.enableLocking();
       });
 
+      testUsingContext('disposing device disposes the portForwarder', () async {
+        final IOSDevice device = IOSDevice('123');
+        device.portForwarder = mockPortForwarder;
+        device.setLogReader(mockApp, mockLogReader);
+        await device.dispose();
+        verify(mockPortForwarder.dispose()).called(1);
+      }, overrides: <Type, Generator>{
+        Platform: () => macPlatform,
+      });
+
       testUsingContext('returns failed if the IOSDevice is not found', () async {
         final IOSDevice device = IOSDevice('123');
         when(mockIMobileDevice.getInfoForDevice(any, 'CPUArchitecture')).thenThrow(

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

@@ -381,4 +381,7 @@ class MockPortForwarder extends DevicePortForwarder {
   Future<void> unforward(ForwardedPort forwardedPort) {
     throw 'not implemented';
   }
+
+  @override
+  Future<void> dispose() async {}
 }

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

@@ -550,6 +550,33 @@ void main() {
     expect(await fs.file('foo').readAsString(), testUri.toString());
   }));
 
+  test('HotRunner unforwards device ports', () => testbed.run(() async {
+    final MockDevicePortForwarder mockPortForwarder = MockDevicePortForwarder();
+    when(mockDevice.portForwarder).thenReturn(mockPortForwarder);
+    fs.file(fs.path.join('lib', 'main.dart')).createSync(recursive: true);
+    residentRunner = HotRunner(
+      <FlutterDevice>[
+        mockFlutterDevice,
+      ],
+      stayResident: false,
+      debuggingOptions: DebuggingOptions.enabled(BuildInfo.debug),
+    );
+    when(mockFlutterDevice.runHot(
+      hotRunner: anyNamed('hotRunner'),
+      route: anyNamed('route'),
+    )).thenAnswer((Invocation invocation) async {
+      return 0;
+    });
+
+    when(mockDevice.dispose()).thenAnswer((Invocation invocation) async {
+      await mockDevice.portForwarder.dispose();
+    });
+
+    await residentRunner.run();
+
+    verify(mockPortForwarder.dispose()).called(1);
+  }));
+
   test('HotRunner handles failure to write vmservice file', () => testbed.run(() async {
     fs.file(fs.path.join('lib', 'main.dart')).createSync(recursive: true);
     residentRunner = HotRunner(
@@ -650,6 +677,7 @@ class MockDevFS extends Mock implements DevFS {}
 class MockIsolate extends Mock implements Isolate {}
 class MockDevice extends Mock implements Device {}
 class MockDeviceLogReader extends Mock implements DeviceLogReader {}
+class MockDevicePortForwarder extends Mock implements DevicePortForwarder {}
 class MockUsage extends Mock implements Usage {}
 class MockProcessManager extends Mock implements ProcessManager {}
 class MockServiceEvent extends Mock implements ServiceEvent {}