From b6fef5cfda2d32eefd06b873490f234a167884ed Mon Sep 17 00:00:00 2001 From: Jenn Magder Date: Tue, 5 Nov 2024 11:01:01 -0800 Subject: [PATCH] Kill interactive script job `xcdevice observe` processes on tool/daemon shutdown (#157646) To convince `xcdevice observe` to redirect to stdout it's being launched in an interactive shell `/usr/bin/script -t 0 /dev/null /usr/bin/arch -arm64e xcrun xcdevice observe --usb` https://github.com/flutter/flutter/blob/1cc8a07ace699de12fcd31ac9e8fcfa14f1017e2/packages/flutter_tools/lib/src/macos/xcdevice.dart#L261-L263 When the `flutter` command exits, the interactive script process is terminated, but not its jobs `xcdevice observe --usb`. Instead of `-9` sigterm killing the interactive script, send it a [`SIGHUP`](https://linux.die.net/Bash-Beginners-Guide/sect_12_01.html#sect_12_01_01_02) (signal hangup) during `XCDevice.dispose()`, which will exit its processes. Add a shutdown hook to ensure `dispose` is run when the process exits. Fixes https://github.com/flutter/flutter/issues/73859 --- packages/flutter_tools/lib/src/base/io.dart | 1 + .../flutter_tools/lib/src/context_runner.dart | 1 + .../flutter_tools/lib/src/macos/xcdevice.dart | 21 ++++++++--- .../test/general.shard/macos/xcode_test.dart | 37 +++++++++++++++++++ 4 files changed, 54 insertions(+), 6 deletions(-) diff --git a/packages/flutter_tools/lib/src/base/io.dart b/packages/flutter_tools/lib/src/base/io.dart index 9ebe64932a9..0dc408fb64b 100644 --- a/packages/flutter_tools/lib/src/base/io.dart +++ b/packages/flutter_tools/lib/src/base/io.dart @@ -174,6 +174,7 @@ class ProcessSignal { const ProcessSignal(this._delegate, {@visibleForTesting Platform platform = const LocalPlatform()}) : _platform = platform; + static const ProcessSignal sighup = PosixProcessSignal(io.ProcessSignal.sighup); static const ProcessSignal sigwinch = PosixProcessSignal(io.ProcessSignal.sigwinch); static const ProcessSignal sigterm = PosixProcessSignal(io.ProcessSignal.sigterm); static const ProcessSignal sigusr1 = PosixProcessSignal(io.ProcessSignal.sigusr1); diff --git a/packages/flutter_tools/lib/src/context_runner.dart b/packages/flutter_tools/lib/src/context_runner.dart index 7c73f2ab5dc..cd6755d8509 100644 --- a/packages/flutter_tools/lib/src/context_runner.dart +++ b/packages/flutter_tools/lib/src/context_runner.dart @@ -376,6 +376,7 @@ Future runInContext( ), fileSystem: globals.fs, analytics: globals.analytics, + shutdownHooks: globals.shutdownHooks, ), XcodeProjectInterpreter: () => XcodeProjectInterpreter( logger: globals.logger, diff --git a/packages/flutter_tools/lib/src/macos/xcdevice.dart b/packages/flutter_tools/lib/src/macos/xcdevice.dart index f122b6efd5a..b31c9b5fa0a 100644 --- a/packages/flutter_tools/lib/src/macos/xcdevice.dart +++ b/packages/flutter_tools/lib/src/macos/xcdevice.dart @@ -71,6 +71,7 @@ class XCDevice { required IProxy iproxy, required FileSystem fileSystem, required Analytics analytics, + required ShutdownHooks shutdownHooks, @visibleForTesting IOSCoreDeviceControl? coreDeviceControl, XcodeDebug? xcodeDebug, @@ -105,12 +106,13 @@ class XCDevice { _xcode = xcode, _analytics = analytics { + shutdownHooks.addShutdownHook(dispose); + _setupDeviceIdentifierByEventStream(); } void dispose() { - _usbDeviceObserveProcess?.kill(); - _wifiDeviceObserveProcess?.kill(); + _stopObservingTetheredIOSDevices(); _usbDeviceWaitProcess?.kill(); _wifiDeviceWaitProcess?.kill(); } @@ -227,13 +229,13 @@ class XCDevice { final Future usbProcessExited = _usbDeviceObserveProcess!.exitCode.then((int status) { _logger.printTrace('xcdevice observe --usb exited with code $exitCode'); // Kill other process in case only one was killed. - _wifiDeviceObserveProcess?.kill(); + _stopObservingTetheredIOSDevices(); }); final Future wifiProcessExited = _wifiDeviceObserveProcess!.exitCode.then((int status) { _logger.printTrace('xcdevice observe --wifi exited with code $exitCode'); // Kill other process in case only one was killed. - _usbDeviceObserveProcess?.kill(); + _stopObservingTetheredIOSDevices(); }); unawaited(Future.wait(>[ @@ -332,8 +334,15 @@ class XCDevice { } void _stopObservingTetheredIOSDevices() { - _usbDeviceObserveProcess?.kill(); - _wifiDeviceObserveProcess?.kill(); + // xcdevice observe is running in an interactive shell. + // Signal script child jobs to exit and exit the shell. + // See https://linux.die.net/Bash-Beginners-Guide/sect_12_01.html#sect_12_01_01_02. + if (_usbDeviceObserveProcess != null) { + ProcessSignal.sighup.kill(_usbDeviceObserveProcess!); + } + if (_wifiDeviceObserveProcess != null) { + ProcessSignal.sighup.kill(_wifiDeviceObserveProcess!); + } } XCDeviceEventNotification? _processXCDeviceStdOut( diff --git a/packages/flutter_tools/test/general.shard/macos/xcode_test.dart b/packages/flutter_tools/test/general.shard/macos/xcode_test.dart index aaaa81f72be..31defb617cc 100644 --- a/packages/flutter_tools/test/general.shard/macos/xcode_test.dart +++ b/packages/flutter_tools/test/general.shard/macos/xcode_test.dart @@ -9,6 +9,7 @@ import 'package:flutter_tools/src/artifacts.dart'; import 'package:flutter_tools/src/base/io.dart' show ProcessException; import 'package:flutter_tools/src/base/logger.dart'; import 'package:flutter_tools/src/base/platform.dart'; +import 'package:flutter_tools/src/base/process.dart'; import 'package:flutter_tools/src/base/user_messages.dart'; import 'package:flutter_tools/src/base/version.dart'; import 'package:flutter_tools/src/build_info.dart'; @@ -539,6 +540,7 @@ void main() { coreDeviceControl: FakeIOSCoreDeviceControl(), xcodeDebug: FakeXcodeDebug(), analytics: const NoOpAnalytics(), + shutdownHooks: FakeShutdownHooks(), ); }); @@ -553,6 +555,35 @@ void main() { }); }); + testWithoutContext('shutdown hooks disposes xcdevice observers', () async { + final ShutdownHooks shutdownHooks = ShutdownHooks(); + + final XCDevice xcdevice = XCDevice( + processManager: FakeProcessManager.any(), + logger: logger, + xcode: Xcode.test(processManager: FakeProcessManager.any()), + platform: FakePlatform(operatingSystem: 'macos'), + artifacts: Artifacts.test(), + cache: Cache.test(processManager: FakeProcessManager.any()), + iproxy: IProxy.test(logger: logger, processManager: FakeProcessManager.any()), + fileSystem: MemoryFileSystem.test(), + coreDeviceControl: FakeIOSCoreDeviceControl(), + xcodeDebug: FakeXcodeDebug(), + analytics: const NoOpAnalytics(), + shutdownHooks: shutdownHooks, + ); + + expect(shutdownHooks.registeredHooks, hasLength(1)); + final Completer doneCompleter = Completer(); + xcdevice.observedDeviceEvents()!.listen(null, onDone: () { + doneCompleter.complete(); + }); + await doneCompleter.future; + xcdevice.dispose(); + expect(logger.traceText, contains('xcdevice observe --usb exited with code 0')); + expect(logger.traceText, contains('xcdevice observe --wifi exited with code 0')); + }); + group('xcdevice', () { late XCDevice xcdevice; late Xcode xcode; @@ -580,6 +611,7 @@ void main() { coreDeviceControl: coreDeviceControl, xcodeDebug: FakeXcodeDebug(), analytics: fakeAnalytics, + shutdownHooks: FakeShutdownHooks(), ); }); @@ -1688,6 +1720,11 @@ class FakeXcodeProjectInterpreter extends Fake implements XcodeProjectInterprete class FakeXcodeDebug extends Fake implements XcodeDebug {} +class FakeShutdownHooks extends Fake implements ShutdownHooks { + @override + void addShutdownHook(ShutdownHook shutdownHook) {} +} + class FakeIOSCoreDeviceControl extends Fake implements IOSCoreDeviceControl { List devices = [];