Skip to content

Commit b6fef5c

Browse files
authored
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 flutter/flutter#73859
1 parent c1b7740 commit b6fef5c

File tree

4 files changed

+54
-6
lines changed

4 files changed

+54
-6
lines changed

packages/flutter_tools/lib/src/base/io.dart

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -174,6 +174,7 @@ class ProcessSignal {
174174
const ProcessSignal(this._delegate, {@visibleForTesting Platform platform = const LocalPlatform()})
175175
: _platform = platform;
176176

177+
static const ProcessSignal sighup = PosixProcessSignal(io.ProcessSignal.sighup);
177178
static const ProcessSignal sigwinch = PosixProcessSignal(io.ProcessSignal.sigwinch);
178179
static const ProcessSignal sigterm = PosixProcessSignal(io.ProcessSignal.sigterm);
179180
static const ProcessSignal sigusr1 = PosixProcessSignal(io.ProcessSignal.sigusr1);

packages/flutter_tools/lib/src/context_runner.dart

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -376,6 +376,7 @@ Future<T> runInContext<T>(
376376
),
377377
fileSystem: globals.fs,
378378
analytics: globals.analytics,
379+
shutdownHooks: globals.shutdownHooks,
379380
),
380381
XcodeProjectInterpreter: () => XcodeProjectInterpreter(
381382
logger: globals.logger,

packages/flutter_tools/lib/src/macos/xcdevice.dart

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,7 @@ class XCDevice {
7171
required IProxy iproxy,
7272
required FileSystem fileSystem,
7373
required Analytics analytics,
74+
required ShutdownHooks shutdownHooks,
7475
@visibleForTesting
7576
IOSCoreDeviceControl? coreDeviceControl,
7677
XcodeDebug? xcodeDebug,
@@ -105,12 +106,13 @@ class XCDevice {
105106
_xcode = xcode,
106107
_analytics = analytics {
107108

109+
shutdownHooks.addShutdownHook(dispose);
110+
108111
_setupDeviceIdentifierByEventStream();
109112
}
110113

111114
void dispose() {
112-
_usbDeviceObserveProcess?.kill();
113-
_wifiDeviceObserveProcess?.kill();
115+
_stopObservingTetheredIOSDevices();
114116
_usbDeviceWaitProcess?.kill();
115117
_wifiDeviceWaitProcess?.kill();
116118
}
@@ -227,13 +229,13 @@ class XCDevice {
227229
final Future<void> usbProcessExited = _usbDeviceObserveProcess!.exitCode.then((int status) {
228230
_logger.printTrace('xcdevice observe --usb exited with code $exitCode');
229231
// Kill other process in case only one was killed.
230-
_wifiDeviceObserveProcess?.kill();
232+
_stopObservingTetheredIOSDevices();
231233
});
232234

233235
final Future<void> wifiProcessExited = _wifiDeviceObserveProcess!.exitCode.then((int status) {
234236
_logger.printTrace('xcdevice observe --wifi exited with code $exitCode');
235237
// Kill other process in case only one was killed.
236-
_usbDeviceObserveProcess?.kill();
238+
_stopObservingTetheredIOSDevices();
237239
});
238240

239241
unawaited(Future.wait(<Future<void>>[
@@ -332,8 +334,15 @@ class XCDevice {
332334
}
333335

334336
void _stopObservingTetheredIOSDevices() {
335-
_usbDeviceObserveProcess?.kill();
336-
_wifiDeviceObserveProcess?.kill();
337+
// xcdevice observe is running in an interactive shell.
338+
// Signal script child jobs to exit and exit the shell.
339+
// See https://linux.die.net/Bash-Beginners-Guide/sect_12_01.html#sect_12_01_01_02.
340+
if (_usbDeviceObserveProcess != null) {
341+
ProcessSignal.sighup.kill(_usbDeviceObserveProcess!);
342+
}
343+
if (_wifiDeviceObserveProcess != null) {
344+
ProcessSignal.sighup.kill(_wifiDeviceObserveProcess!);
345+
}
337346
}
338347

339348
XCDeviceEventNotification? _processXCDeviceStdOut(

packages/flutter_tools/test/general.shard/macos/xcode_test.dart

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import 'package:flutter_tools/src/artifacts.dart';
99
import 'package:flutter_tools/src/base/io.dart' show ProcessException;
1010
import 'package:flutter_tools/src/base/logger.dart';
1111
import 'package:flutter_tools/src/base/platform.dart';
12+
import 'package:flutter_tools/src/base/process.dart';
1213
import 'package:flutter_tools/src/base/user_messages.dart';
1314
import 'package:flutter_tools/src/base/version.dart';
1415
import 'package:flutter_tools/src/build_info.dart';
@@ -539,6 +540,7 @@ void main() {
539540
coreDeviceControl: FakeIOSCoreDeviceControl(),
540541
xcodeDebug: FakeXcodeDebug(),
541542
analytics: const NoOpAnalytics(),
543+
shutdownHooks: FakeShutdownHooks(),
542544
);
543545
});
544546

@@ -553,6 +555,35 @@ void main() {
553555
});
554556
});
555557

558+
testWithoutContext('shutdown hooks disposes xcdevice observers', () async {
559+
final ShutdownHooks shutdownHooks = ShutdownHooks();
560+
561+
final XCDevice xcdevice = XCDevice(
562+
processManager: FakeProcessManager.any(),
563+
logger: logger,
564+
xcode: Xcode.test(processManager: FakeProcessManager.any()),
565+
platform: FakePlatform(operatingSystem: 'macos'),
566+
artifacts: Artifacts.test(),
567+
cache: Cache.test(processManager: FakeProcessManager.any()),
568+
iproxy: IProxy.test(logger: logger, processManager: FakeProcessManager.any()),
569+
fileSystem: MemoryFileSystem.test(),
570+
coreDeviceControl: FakeIOSCoreDeviceControl(),
571+
xcodeDebug: FakeXcodeDebug(),
572+
analytics: const NoOpAnalytics(),
573+
shutdownHooks: shutdownHooks,
574+
);
575+
576+
expect(shutdownHooks.registeredHooks, hasLength(1));
577+
final Completer<void> doneCompleter = Completer<void>();
578+
xcdevice.observedDeviceEvents()!.listen(null, onDone: () {
579+
doneCompleter.complete();
580+
});
581+
await doneCompleter.future;
582+
xcdevice.dispose();
583+
expect(logger.traceText, contains('xcdevice observe --usb exited with code 0'));
584+
expect(logger.traceText, contains('xcdevice observe --wifi exited with code 0'));
585+
});
586+
556587
group('xcdevice', () {
557588
late XCDevice xcdevice;
558589
late Xcode xcode;
@@ -580,6 +611,7 @@ void main() {
580611
coreDeviceControl: coreDeviceControl,
581612
xcodeDebug: FakeXcodeDebug(),
582613
analytics: fakeAnalytics,
614+
shutdownHooks: FakeShutdownHooks(),
583615
);
584616
});
585617

@@ -1688,6 +1720,11 @@ class FakeXcodeProjectInterpreter extends Fake implements XcodeProjectInterprete
16881720

16891721
class FakeXcodeDebug extends Fake implements XcodeDebug {}
16901722

1723+
class FakeShutdownHooks extends Fake implements ShutdownHooks {
1724+
@override
1725+
void addShutdownHook(ShutdownHook shutdownHook) {}
1726+
}
1727+
16911728
class FakeIOSCoreDeviceControl extends Fake implements IOSCoreDeviceControl {
16921729

16931730
List<FakeIOSCoreDevice> devices = <FakeIOSCoreDevice>[];

0 commit comments

Comments
 (0)