From fac41dde7f15fbc3aa317071bd641659c5653e44 Mon Sep 17 00:00:00 2001 From: Christopher Fujino Date: Mon, 11 Dec 2023 16:32:18 -0800 Subject: [PATCH] [flutter_tools] catch SocketException writing to ios-deploy stdin (#139784) Fixes https://github.com/flutter/flutter/issues/139709 This adds a static helper method `ProcessUtils.writelnToStdinGuarded()`, which will asynchronously write to a sub-process's STDIN `IOSink` and catch errors. In talking with Brian, it sounds like this is the best and most reliable way to catch `SocketException`s during these writes *to sub-process file descriptors* specifically (with a "real" hard drive file, the future returned by `.flush()` should complete with the write error). Also, as I note in the dartdoc to `writelnToStdinGuarded()`, the behavior seems to be different between macOS and linux. Moving forward, in any place where we want to catch exceptions writing to STDIN, we will want to use this new helper. --- .../flutter_tools/lib/src/base/process.dart | 46 +++++++++++++ .../flutter_tools/lib/src/ios/ios_deploy.dart | 67 ++++++++++++++----- .../general.shard/ios/ios_deploy_test.dart | 44 ++++++++++-- .../ios/ios_device_logger_test.dart | 2 +- 4 files changed, 139 insertions(+), 20 deletions(-) diff --git a/packages/flutter_tools/lib/src/base/process.dart b/packages/flutter_tools/lib/src/base/process.dart index d1372093bdf..d0acf2b5d8b 100644 --- a/packages/flutter_tools/lib/src/base/process.dart +++ b/packages/flutter_tools/lib/src/base/process.dart @@ -233,6 +233,52 @@ abstract class ProcessUtils { List cli, { Map? environment, }); + + /// Write [line] to [stdin] and catch any errors with [onError]. + /// + /// Specifically with [Process] file descriptors, an exception that is + /// thrown as part of a write can be most reliably caught with a + /// [ZoneSpecification] error handler. + /// + /// On some platforms, the following code appears to work: + /// + /// ```dart + /// stdin.writeln(line); + /// try { + /// await stdin.flush(line); + /// } catch (err) { + /// // handle error + /// } + /// ``` + /// + /// However it did not catch a [SocketException] on Linux. + static Future writelnToStdinGuarded({ + required IOSink stdin, + required String line, + required void Function(Object, StackTrace) onError, + }) async { + final Completer completer = Completer(); + + void writeFlushAndComplete() { + stdin.writeln(line); + stdin.flush().whenComplete(() { + if (!completer.isCompleted) { + completer.complete(); + } + }); + } + + runZonedGuarded( + writeFlushAndComplete, + (Object error, StackTrace stackTrace) { + onError(error, stackTrace); + if (!completer.isCompleted) { + completer.complete(); + } + }, + ); + return completer.future; + } } class _DefaultProcessUtils implements ProcessUtils { diff --git a/packages/flutter_tools/lib/src/ios/ios_deploy.dart b/packages/flutter_tools/lib/src/ios/ios_deploy.dart index 71cb6dbee63..a4cf23ffa54 100644 --- a/packages/flutter_tools/lib/src/ios/ios_deploy.dart +++ b/packages/flutter_tools/lib/src/ios/ios_deploy.dart @@ -598,29 +598,66 @@ class IOSDeployDebugger { if (!debuggerAttached) { return; } - try { - // Stop the app, which will prompt the backtrace to be printed for all threads in the stdoutSubscription handler. - _iosDeployProcess?.stdin.writeln(_signalStop); - } on SocketException catch (error) { - // Best effort, try to detach, but maybe the app already exited or already detached. - _logger.printTrace('Could not stop app from debugger: $error'); - } + // Stop the app, which will prompt the backtrace to be printed for all + // threads in the stdoutSubscription handler. + await stdinWriteln( + _signalStop, + onError: (Object error, _) { + _logger.printTrace('Could not stop the app: $error'); + }, + ); + // Wait for logging to finish on process exit. return logLines.drain(); } - void detach() { + Future? _stdinWriteFuture; + + /// Queue write of [line] to STDIN of [_iosDeployProcess]. + /// + /// No-op if [_iosDeployProcess] is null. + /// + /// This write will not happen until the flush of any previous writes have + /// completed, because calling [IOSink.flush()] before a previous flush has + /// completed will throw a [StateError]. + /// + /// This method needs to keep track of the [_stdinWriteFuture] from previous + /// calls because the future returned by [detach] is not always await-ed. + Future stdinWriteln(String line, {required void Function(Object, StackTrace) onError}) async { + final Process? process = _iosDeployProcess; + if (process == null) { + return; + } + + Future writeln() { + return ProcessUtils.writelnToStdinGuarded( + stdin: process.stdin, + line: line, + onError: onError, + ); + } + + if (_stdinWriteFuture != null) { + _stdinWriteFuture = _stdinWriteFuture!.then((_) => writeln()); + } else { + _stdinWriteFuture = writeln(); + } + + return _stdinWriteFuture; + } + + Future detach() async { if (!debuggerAttached) { return; } - try { - // Detach lldb from the app process. - _iosDeployProcess?.stdin.writeln('process detach'); - } on SocketException catch (error) { - // Best effort, try to detach, but maybe the app already exited or already detached. - _logger.printTrace('Could not detach from debugger: $error'); - } + return stdinWriteln( + 'process detach', + onError: (Object error, _) { + // Best effort, try to detach, but maybe the app already exited or already detached. + _logger.printTrace('Could not detach from debugger: $error'); + } + ); } } diff --git a/packages/flutter_tools/test/general.shard/ios/ios_deploy_test.dart b/packages/flutter_tools/test/general.shard/ios/ios_deploy_test.dart index 7862b4d9191..7ef5f83c163 100644 --- a/packages/flutter_tools/test/general.shard/ios/ios_deploy_test.dart +++ b/packages/flutter_tools/test/general.shard/ios/ios_deploy_test.dart @@ -9,12 +9,14 @@ import 'package:file/memory.dart'; import 'package:file_testing/file_testing.dart'; import 'package:flutter_tools/src/artifacts.dart'; import 'package:flutter_tools/src/base/file_system.dart'; +import 'package:flutter_tools/src/base/io.dart'; 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/cache.dart'; import 'package:flutter_tools/src/device.dart'; import 'package:flutter_tools/src/ios/ios_deploy.dart'; +import 'package:test/fake.dart'; import '../../src/common.dart'; import '../../src/fake_process_manager.dart'; @@ -386,7 +388,26 @@ void main () { ); expect(stdin.stream.transform(const Utf8Decoder()), emits('process detach')); await iosDeployDebugger.launchAndAttach(); - iosDeployDebugger.detach(); + await iosDeployDebugger.detach(); + }); + + testWithoutContext('detach handles broken pipe', () async { + final StreamSink> stdinSink = _ClosedStdinController(); + final FakeProcessManager processManager = FakeProcessManager.list([ + FakeCommand( + command: const ['ios-deploy'], + stdout: '(lldb) run\nsuccess', + stdin: IOSink(stdinSink), + ), + ]); + final BufferLogger logger = BufferLogger.test(); + final IOSDeployDebugger iosDeployDebugger = IOSDeployDebugger.test( + processManager: processManager, + logger: logger, + ); + await iosDeployDebugger.launchAndAttach(); + await iosDeployDebugger.detach(); + expect(logger.traceText, contains('Could not detach from debugger')); }); testWithoutContext('stop with backtrace', () async { @@ -399,18 +420,28 @@ void main () { ], stdout: '(lldb) run\nsuccess\nLog on attach\n(lldb) Process 6156 stopped\n* thread #1, stop reason = Assertion failed:\n(lldb) Process 6156 detached', - stdin: IOSink(stdin.sink), + stdin: IOSink(stdin), ), ]); final IOSDeployDebugger iosDeployDebugger = IOSDeployDebugger.test( processManager: processManager, ); await iosDeployDebugger.launchAndAttach(); - await iosDeployDebugger.stopAndDumpBacktrace(); - expect(await stdinStream.take(3).toList(), [ + List? stdinLines; + + // These two futures will deadlock if await-ed sequentially + await Future.wait(>[ + iosDeployDebugger.stopAndDumpBacktrace(), + stdinStream.take(5).toList().then( + (List lines) => stdinLines = lines, + ), + ]); + expect(stdinLines, const [ 'thread backtrace all', '\n', 'process detach', + '\n', + 'process signal SIGSTOP', ]); }); @@ -616,6 +647,11 @@ process continue }); } +class _ClosedStdinController extends Fake implements StreamSink> { + @override + Future addStream(Stream> stream) async => throw const SocketException('Bad pipe'); +} + IOSDeploy setUpIOSDeploy(ProcessManager processManager, { Artifacts? artifacts, }) { diff --git a/packages/flutter_tools/test/general.shard/ios/ios_device_logger_test.dart b/packages/flutter_tools/test/general.shard/ios/ios_device_logger_test.dart index 01506cd99df..c113334ba03 100644 --- a/packages/flutter_tools/test/general.shard/ios/ios_device_logger_test.dart +++ b/packages/flutter_tools/test/general.shard/ios/ios_device_logger_test.dart @@ -1001,7 +1001,7 @@ class FakeIOSDeployDebugger extends Fake implements IOSDeployDebugger { Stream logLines = const Stream.empty(); @override - void detach() { + Future detach() async { detached = true; } }