From 72432c3f15b26fe55f8fd822e5fb3581260f75dd Mon Sep 17 00:00:00 2001 From: "auto-submit[bot]" <98614782+auto-submit[bot]@users.noreply.github.com> Date: Wed, 7 Aug 2024 16:11:26 +0000 Subject: [PATCH] Reverts "[tool] Guard process writes to frontend server in `ResidentCompiler` (#152358)" (#153028) Reverts: flutter/flutter#152358 Initiated by: zanderso Reason for reverting: Speculative revert to determine whether this PR is related to https://github.com/flutter/flutter/issues/153026 Original PR Author: andrewkolos Reviewed By: {christopherfujino} This change reverts the following previous change: Contributes to fixing https://github.com/flutter/flutter/issues/137184. Cleaned up version of earlier PR, https://github.com/flutter/flutter/pull/152187. This PR guards all the writes to `Process::stdin` by wrapping them with `ProcessUtils.writelnToStdinUnsafe`. This way, if any writes fail, we should at least get a stacktrace in our crash reporting. --- .../flutter_tools/lib/src/base/process.dart | 15 +- packages/flutter_tools/lib/src/compile.dart | 133 +++++++----------- packages/flutter_tools/lib/src/devfs.dart | 8 +- .../lib/src/isolated/devfs_web.dart | 2 +- .../lib/src/isolated/resident_web_runner.dart | 4 +- .../lib/src/resident_runner.dart | 2 +- packages/flutter_tools/lib/src/run_hot.dart | 4 +- .../flutter_tools/lib/src/test/runner.dart | 2 +- .../lib/src/test/test_compiler.dart | 6 +- .../compile_incremental_test.dart | 2 +- .../test/general.shard/hot_shared.dart | 2 +- .../resident_runner_helpers.dart | 4 +- .../resident_web_runner_test.dart | 4 +- .../test/test_compiler_test.dart | 4 +- 14 files changed, 71 insertions(+), 121 deletions(-) diff --git a/packages/flutter_tools/lib/src/base/process.dart b/packages/flutter_tools/lib/src/base/process.dart index 0ae0a3f601..96df867826 100644 --- a/packages/flutter_tools/lib/src/base/process.dart +++ b/packages/flutter_tools/lib/src/base/process.dart @@ -228,11 +228,8 @@ abstract class ProcessUtils { /// Write [line] to [stdin] and catch any errors with [onError]. /// - /// Concurrent calls to this method will result in an exception due to its - /// dependence on [IOSink.flush] (see https://github.com/dart-lang/sdk/issues/25277). - /// - /// Context: specifically with [Process] file descriptors, an exception that - /// is thrown as part of a write can be most reliably caught with a + /// 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: @@ -281,10 +278,6 @@ abstract class ProcessUtils { ); } - /// See [writelnToStdinGuarded]. - /// - /// In the event that the write or flush fails, this will throw an exception - /// that preserves the stack trace of the callsite. static Future writelnToStdinUnsafe({ required IOSink stdin, required String line, @@ -296,10 +289,6 @@ abstract class ProcessUtils { ); } - /// See [writeToStdinGuarded]. - /// - /// In the event that the write or flush fails, this will throw an exception - /// that preserves the stack trace of the callsite. static Future writeToStdinUnsafe({ required IOSink stdin, required String content, diff --git a/packages/flutter_tools/lib/src/compile.dart b/packages/flutter_tools/lib/src/compile.dart index c575ca7956..f1ce205dbf 100644 --- a/packages/flutter_tools/lib/src/compile.dart +++ b/packages/flutter_tools/lib/src/compile.dart @@ -7,7 +7,6 @@ import 'dart:typed_data'; import 'package:meta/meta.dart'; import 'package:package_config/package_config.dart'; -import 'package:pool/pool.dart'; import 'package:process/process.dart'; import 'package:usage/uuid/uuid.dart'; @@ -17,7 +16,6 @@ import 'base/file_system.dart'; import 'base/io.dart'; import 'base/logger.dart'; import 'base/platform.dart'; -import 'base/process.dart'; import 'build_info.dart'; import 'convert.dart'; @@ -591,7 +589,7 @@ abstract class ResidentCompiler { /// Should be invoked when results of compilation are accepted by the client. /// /// Either [accept] or [reject] should be called after every [recompile] call. - Future accept(); + void accept(); /// Should be invoked when results of compilation are rejected by the client. /// @@ -601,7 +599,7 @@ abstract class ResidentCompiler { /// Should be invoked when frontend server compiler should forget what was /// accepted previously so that next call to [recompile] produces complete /// kernel file. - Future reset(); + void reset(); Future shutdown(); } @@ -744,9 +742,11 @@ class DefaultResidentCompiler implements ResidentCompiler { final String inputKey = Uuid().generateV4(); if (nativeAssets != null && nativeAssets.isNotEmpty) { - await _writelnToServerStdin('native-assets $nativeAssets', printTrace: true); + server.stdin.writeln('native-assets $nativeAssets'); + _logger.printTrace('<- native-assets $nativeAssets'); } - await _writelnToServerStdin('recompile $mainUri $inputKey', printTrace: true); + server.stdin.writeln('recompile $mainUri $inputKey'); + _logger.printTrace('<- recompile $mainUri $inputKey'); final List? invalidatedFiles = request.invalidatedFiles; if (invalidatedFiles != null) { for (final Uri fileUri in invalidatedFiles) { @@ -761,7 +761,8 @@ class DefaultResidentCompiler implements ResidentCompiler { _logger.printTrace(message); } } - await _writelnToServerStdin(inputKey, printTrace: true); + server.stdin.writeln(inputKey); + _logger.printTrace('<- $inputKey'); return _stdoutHandler.compilerOutput?.future; } @@ -898,10 +899,12 @@ class DefaultResidentCompiler implements ResidentCompiler { })); if (nativeAssetsUri != null && nativeAssetsUri.isNotEmpty) { - await _writelnToServerStdin('native assets $nativeAssetsUri', printTrace: true); + _server?.stdin.writeln('native-assets $nativeAssetsUri'); + _logger.printTrace('<- native-assets $nativeAssetsUri'); } - await _writelnToServerStdin('compile $scriptUri', printTrace: true); + _server?.stdin.writeln('compile $scriptUri'); + _logger.printTrace('<- compile $scriptUri'); return _stdoutHandler.compilerOutput?.future; } @@ -942,24 +945,24 @@ class DefaultResidentCompiler implements ResidentCompiler { } final String inputKey = Uuid().generateV4(); - await _writelnToServerStdinAll([ - 'compile-expression $inputKey', - request.expression, - ...?request.definitions, - inputKey, - ...?request.definitionTypes, - inputKey, - ...?request.typeDefinitions, - inputKey, - ...?request.typeBounds, - inputKey, - ...?request.typeDefaults, - inputKey, - request.libraryUri ?? '', - request.klass ?? '', - request.method ?? '', - request.isStatic.toString(), - ]); + server.stdin + ..writeln('compile-expression $inputKey') + ..writeln(request.expression); + request.definitions?.forEach(server.stdin.writeln); + server.stdin.writeln(inputKey); + request.definitionTypes?.forEach(server.stdin.writeln); + server.stdin.writeln(inputKey); + request.typeDefinitions?.forEach(server.stdin.writeln); + server.stdin.writeln(inputKey); + request.typeBounds?.forEach(server.stdin.writeln); + server.stdin.writeln(inputKey); + request.typeDefaults?.forEach(server.stdin.writeln); + server.stdin + ..writeln(inputKey) + ..writeln(request.libraryUri ?? '') + ..writeln(request.klass ?? '') + ..writeln(request.method ?? '') + ..writeln(request.isStatic); return _stdoutHandler.compilerOutput?.future; } @@ -997,28 +1000,27 @@ class DefaultResidentCompiler implements ResidentCompiler { } final String inputKey = Uuid().generateV4(); - await _writelnToServerStdinAll([ - 'compile-expression-to-js $inputKey', - request.libraryUri ?? '', - request.line.toString(), - request.column.toString(), - for (final MapEntry entry in request.jsModules?.entries ?? >[]) - '${entry.key}:${entry.value}', - inputKey, - for (final MapEntry entry in request.jsFrameValues?.entries ?? >[]) - '${entry.key}:${entry.value}', - inputKey, - request.moduleName ?? '', - request.expression ?? '' - ]); + server.stdin + ..writeln('compile-expression-to-js $inputKey') + ..writeln(request.libraryUri ?? '') + ..writeln(request.line) + ..writeln(request.column); + request.jsModules?.forEach((String k, String v) { server.stdin.writeln('$k:$v'); }); + server.stdin.writeln(inputKey); + request.jsFrameValues?.forEach((String k, String v) { server.stdin.writeln('$k:$v'); }); + server.stdin + ..writeln(inputKey) + ..writeln(request.moduleName ?? '') + ..writeln(request.expression ?? ''); return _stdoutHandler.compilerOutput?.future; } @override - Future accept() async { + void accept() { if (_compileRequestNeedsConfirmation) { - await _writelnToServerStdin('accept', printTrace: true); + _server?.stdin.writeln('accept'); + _logger.printTrace('<- accept'); } _compileRequestNeedsConfirmation = false; } @@ -1039,14 +1041,16 @@ class DefaultResidentCompiler implements ResidentCompiler { return Future.value(); } _stdoutHandler.reset(expectSources: false); - await _writelnToServerStdin('reject', printTrace: true); + _server?.stdin.writeln('reject'); + _logger.printTrace('<- reject'); _compileRequestNeedsConfirmation = false; return _stdoutHandler.compilerOutput?.future; } @override - Future reset() async { - await _writelnToServerStdin('reset', printTrace: true); + void reset() { + _server?.stdin.writeln('reset'); + _logger.printTrace('<- reset'); } @override @@ -1060,43 +1064,6 @@ class DefaultResidentCompiler implements ResidentCompiler { server.kill(); return server.exitCode; } - - Future _writelnToServerStdin(String line, { - bool printTrace = false, - }) async { - await _writelnToServerStdinAll([line], printTrace: printTrace); - } - - // TODO(andrewkolos): Concurrent calls to ProcessUtils.writelnToStdinUnsafe - // against the same stdin will result in an exception. To guard against this, - // we need to force calls to run serially. Ideally, this wouldn't be - // necessary since we shouldn't have multiple concurrent writes to the - // compiler process. - // However, we do. See https://github.com/flutter/flutter/issues/152577. - final Pool _serverStdinWritePool = Pool(1); - Future _writelnToServerStdinAll(List lines, { - bool printTrace = false, - }) async { - final Process? server = _server; - if (server == null) { - return; - } - final PoolResource request = await _serverStdinWritePool.request(); - try { - await ProcessUtils.writelnToStdinUnsafe( - stdin: server.stdin, - line: lines.join('\n'), - ); - - for (final String line in lines) { - if (printTrace) { - _logger.printTrace('<- $line'); - } - } - } finally { - request.release(); - } - } } /// Convert a file URI into a multi-root scheme URI if provided, otherwise diff --git a/packages/flutter_tools/lib/src/devfs.dart b/packages/flutter_tools/lib/src/devfs.dart index f5f4ed3517..a38e9824d1 100644 --- a/packages/flutter_tools/lib/src/devfs.dart +++ b/packages/flutter_tools/lib/src/devfs.dart @@ -585,7 +585,7 @@ class DevFS { bool assetBuildFailed = false; int syncedBytes = 0; if (fullRestart) { - await generator.reset(); + generator.reset(); } // On a full restart, or on an initial compile for the attach based workflow, // this will produce a full dill. Subsequent invocations will produce incremental @@ -614,12 +614,6 @@ class DevFS { // before processing bundle. _logger.printTrace('Processing bundle.'); // await null to give time for telling the compiler to compile. - // TODO(andrewkolos): This is a hack. Adding any more awaits to the compiler's - // recompile method will cause this to be insufficent. - // https://github.com/flutter/flutter/issues/151255. - await null; - await null; - await null; await null; // The tool writes the assets into the AssetBundle working dir so that they diff --git a/packages/flutter_tools/lib/src/isolated/devfs_web.dart b/packages/flutter_tools/lib/src/isolated/devfs_web.dart index dc7c529921..690278ba67 100644 --- a/packages/flutter_tools/lib/src/isolated/devfs_web.dart +++ b/packages/flutter_tools/lib/src/isolated/devfs_web.dart @@ -1020,7 +1020,7 @@ class WebDevFS implements DevFS { await _validateTemplateFile('flutter_bootstrap.js'); final DateTime candidateCompileTime = DateTime.now(); if (fullRestart) { - await generator.reset(); + generator.reset(); } // The tool generates an entrypoint file in a temp directory to handle diff --git a/packages/flutter_tools/lib/src/isolated/resident_web_runner.dart b/packages/flutter_tools/lib/src/isolated/resident_web_runner.dart index 3a9d210a2b..c212a0ed63 100644 --- a/packages/flutter_tools/lib/src/isolated/resident_web_runner.dart +++ b/packages/flutter_tools/lib/src/isolated/resident_web_runner.dart @@ -334,7 +334,7 @@ Please provide a valid TCP port (an integer between 0 and 65535, inclusive). appFailedToStart(); return 1; } - await device!.generator!.accept(); + device!.generator!.accept(); cacheInitialDillCompilation(); } else { final WebBuilder webBuilder = WebBuilder( @@ -418,7 +418,7 @@ Please provide a valid TCP port (an integer between 0 and 65535, inclusive). // Full restart is always false for web, since the extra recompile is wasteful. final UpdateFSReport report = await _updateDevFS(); if (report.success) { - await device!.generator!.accept(); + device!.generator!.accept(); } else { status.stop(); await device!.generator!.reject(); diff --git a/packages/flutter_tools/lib/src/resident_runner.dart b/packages/flutter_tools/lib/src/resident_runner.dart index eefcf4ca3f..daabad8b91 100644 --- a/packages/flutter_tools/lib/src/resident_runner.dart +++ b/packages/flutter_tools/lib/src/resident_runner.dart @@ -595,7 +595,7 @@ class FlutterDevice { Future updateReloadStatus(bool wasReloadSuccessful) async { if (wasReloadSuccessful) { - await generator?.accept(); + generator?.accept(); } else { await generator?.reject(); } diff --git a/packages/flutter_tools/lib/src/run_hot.dart b/packages/flutter_tools/lib/src/run_hot.dart index a8084cbb20..c73e578df0 100644 --- a/packages/flutter_tools/lib/src/run_hot.dart +++ b/packages/flutter_tools/lib/src/run_hot.dart @@ -298,7 +298,7 @@ class HotRunner extends ResidentRunner { // VM must have accepted the kernel binary, there will be no reload // report, so we let incremental compiler know that source code was accepted. if (device!.generator != null) { - await device.generator!.accept(); + device.generator!.accept(); } final List views = await device.vmService!.getFlutterViews(); for (final FlutterView view in views) { @@ -626,7 +626,7 @@ class HotRunner extends ResidentRunner { // VM must have accepted the kernel binary, there will be no reload // report, so we let incremental compiler know that source code was accepted. if (device!.generator != null) { - await device.generator!.accept(); + device.generator!.accept(); } } // Check if the isolate is paused and resume it. diff --git a/packages/flutter_tools/lib/src/test/runner.dart b/packages/flutter_tools/lib/src/test/runner.dart index 3181ef676b..7bbadbb918 100644 --- a/packages/flutter_tools/lib/src/test/runner.dart +++ b/packages/flutter_tools/lib/src/test/runner.dart @@ -647,7 +647,7 @@ class SpawnPlugin extends PlatformPlugin { fs: globals.fs, nativeAssetsYaml: nativeAssetsYaml, ); - await residentCompiler.accept(); + residentCompiler.accept(); globals.printTrace('Compiling ${sourceFile.absolute.uri} took ${compilerTime.elapsedMilliseconds}ms'); testTimeRecorder?.stop(TestTimePhases.Compile, testTimeRecorderStopwatch!); diff --git a/packages/flutter_tools/lib/src/test/test_compiler.dart b/packages/flutter_tools/lib/src/test/test_compiler.dart index e1ac7d8e5c..9c09ba92c4 100644 --- a/packages/flutter_tools/lib/src/test/test_compiler.dart +++ b/packages/flutter_tools/lib/src/test/test_compiler.dart @@ -188,11 +188,9 @@ class TestCompiler { // compiler to avoid reusing compiler that might have gotten into // a weird state. if (outputPath == null || compilerOutput!.errorCount > 0) { - await _shutdown(); request.result.complete(); + await _shutdown(); } else { - await compiler!.accept(); - await compiler!.reset(); if (shouldCopyDillFile) { final String path = request.mainUri.toFilePath(windows: globals.platform.isWindows); final File outputFile = globals.fs.file(outputPath); @@ -211,6 +209,8 @@ class TestCompiler { } else { request.result.complete(outputPath); } + compiler!.accept(); + compiler!.reset(); } globals.printTrace('Compiling ${request.mainUri} took ${compilerTime.elapsedMilliseconds}ms'); testTimeRecorder?.stop(TestTimePhases.Compile, testTimeRecorderStopwatch!); diff --git a/packages/flutter_tools/test/general.shard/compile_incremental_test.dart b/packages/flutter_tools/test/general.shard/compile_incremental_test.dart index 47c2a956fd..14749ae1d4 100644 --- a/packages/flutter_tools/test/general.shard/compile_incremental_test.dart +++ b/packages/flutter_tools/test/general.shard/compile_incremental_test.dart @@ -521,7 +521,7 @@ Future _accept( MemoryIOSink frontendServerStdIn, String expected, ) async { - await generator.accept(); + generator.accept(); final String commands = frontendServerStdIn.getAndClear(); final RegExp re = RegExp(expected); expect(commands, matches(re)); diff --git a/packages/flutter_tools/test/general.shard/hot_shared.dart b/packages/flutter_tools/test/general.shard/hot_shared.dart index c76f5397f1..39f0aa0da5 100644 --- a/packages/flutter_tools/test/general.shard/hot_shared.dart +++ b/packages/flutter_tools/test/general.shard/hot_shared.dart @@ -192,7 +192,7 @@ class TestHotRunnerConfig extends HotRunnerConfig { class FakeResidentCompiler extends Fake implements ResidentCompiler { @override - Future accept() async {} + void accept() {} } class FakeFlutterVmService extends Fake implements FlutterVmService { diff --git a/packages/flutter_tools/test/general.shard/resident_runner_helpers.dart b/packages/flutter_tools/test/general.shard/resident_runner_helpers.dart index 3bab03eb44..165c00d234 100644 --- a/packages/flutter_tools/test/general.shard/resident_runner_helpers.dart +++ b/packages/flutter_tools/test/general.shard/resident_runner_helpers.dart @@ -346,10 +346,10 @@ class FakeResidentCompiler extends Fake implements ResidentCompiler { } @override - Future accept() async {} + void accept() { } @override - Future reset() async {} + void reset() { } } class FakeProjectFileInvalidator extends Fake implements ProjectFileInvalidator { diff --git a/packages/flutter_tools/test/general.shard/resident_web_runner_test.dart b/packages/flutter_tools/test/general.shard/resident_web_runner_test.dart index 67858cad08..563b0eac30 100644 --- a/packages/flutter_tools/test/general.shard/resident_web_runner_test.dart +++ b/packages/flutter_tools/test/general.shard/resident_web_runner_test.dart @@ -1508,10 +1508,10 @@ class FakeResidentCompiler extends Fake implements ResidentCompiler { } @override - Future accept() async {} + void accept() {} @override - Future reset() async {} + void reset() {} @override Future reject() async { diff --git a/packages/flutter_tools/test/general.shard/test/test_compiler_test.dart b/packages/flutter_tools/test/general.shard/test/test_compiler_test.dart index 9585fbe924..257ad4f999 100644 --- a/packages/flutter_tools/test/general.shard/test/test_compiler_test.dart +++ b/packages/flutter_tools/test/general.shard/test/test_compiler_test.dart @@ -244,10 +244,10 @@ class FakeResidentCompiler extends Fake implements ResidentCompiler { } @override - Future accept() async {} + void accept() { } @override - Future reset() async {} + void reset() { } @override Future shutdown() async {