diff --git a/packages/flutter_tools/lib/src/base/process.dart b/packages/flutter_tools/lib/src/base/process.dart index 96df867826..0ae0a3f601 100644 --- a/packages/flutter_tools/lib/src/base/process.dart +++ b/packages/flutter_tools/lib/src/base/process.dart @@ -228,8 +228,11 @@ abstract class ProcessUtils { /// 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 + /// 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 /// [ZoneSpecification] error handler. /// /// On some platforms, the following code appears to work: @@ -278,6 +281,10 @@ 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, @@ -289,6 +296,10 @@ 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 f1ce205dbf..c575ca7956 100644 --- a/packages/flutter_tools/lib/src/compile.dart +++ b/packages/flutter_tools/lib/src/compile.dart @@ -7,6 +7,7 @@ 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'; @@ -16,6 +17,7 @@ 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'; @@ -589,7 +591,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. - void accept(); + Future accept(); /// Should be invoked when results of compilation are rejected by the client. /// @@ -599,7 +601,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. - void reset(); + Future reset(); Future shutdown(); } @@ -742,11 +744,9 @@ class DefaultResidentCompiler implements ResidentCompiler { final String inputKey = Uuid().generateV4(); if (nativeAssets != null && nativeAssets.isNotEmpty) { - server.stdin.writeln('native-assets $nativeAssets'); - _logger.printTrace('<- native-assets $nativeAssets'); + await _writelnToServerStdin('native-assets $nativeAssets', printTrace: true); } - server.stdin.writeln('recompile $mainUri $inputKey'); - _logger.printTrace('<- recompile $mainUri $inputKey'); + await _writelnToServerStdin('recompile $mainUri $inputKey', printTrace: true); final List? invalidatedFiles = request.invalidatedFiles; if (invalidatedFiles != null) { for (final Uri fileUri in invalidatedFiles) { @@ -761,8 +761,7 @@ class DefaultResidentCompiler implements ResidentCompiler { _logger.printTrace(message); } } - server.stdin.writeln(inputKey); - _logger.printTrace('<- $inputKey'); + await _writelnToServerStdin(inputKey, printTrace: true); return _stdoutHandler.compilerOutput?.future; } @@ -899,12 +898,10 @@ class DefaultResidentCompiler implements ResidentCompiler { })); if (nativeAssetsUri != null && nativeAssetsUri.isNotEmpty) { - _server?.stdin.writeln('native-assets $nativeAssetsUri'); - _logger.printTrace('<- native-assets $nativeAssetsUri'); + await _writelnToServerStdin('native assets $nativeAssetsUri', printTrace: true); } - _server?.stdin.writeln('compile $scriptUri'); - _logger.printTrace('<- compile $scriptUri'); + await _writelnToServerStdin('compile $scriptUri', printTrace: true); return _stdoutHandler.compilerOutput?.future; } @@ -945,24 +942,24 @@ class DefaultResidentCompiler implements ResidentCompiler { } final String inputKey = Uuid().generateV4(); - 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); + 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(), + ]); return _stdoutHandler.compilerOutput?.future; } @@ -1000,27 +997,28 @@ class DefaultResidentCompiler implements ResidentCompiler { } final String inputKey = Uuid().generateV4(); - 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 ?? ''); + 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 ?? '' + ]); return _stdoutHandler.compilerOutput?.future; } @override - void accept() { + Future accept() async { if (_compileRequestNeedsConfirmation) { - _server?.stdin.writeln('accept'); - _logger.printTrace('<- accept'); + await _writelnToServerStdin('accept', printTrace: true); } _compileRequestNeedsConfirmation = false; } @@ -1041,16 +1039,14 @@ class DefaultResidentCompiler implements ResidentCompiler { return Future.value(); } _stdoutHandler.reset(expectSources: false); - _server?.stdin.writeln('reject'); - _logger.printTrace('<- reject'); + await _writelnToServerStdin('reject', printTrace: true); _compileRequestNeedsConfirmation = false; return _stdoutHandler.compilerOutput?.future; } @override - void reset() { - _server?.stdin.writeln('reset'); - _logger.printTrace('<- reset'); + Future reset() async { + await _writelnToServerStdin('reset', printTrace: true); } @override @@ -1064,6 +1060,43 @@ 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 a38e9824d1..f5f4ed3517 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) { - generator.reset(); + await 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,6 +614,12 @@ 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 690278ba67..dc7c529921 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) { - generator.reset(); + await 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 c212a0ed63..3a9d210a2b 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; } - device!.generator!.accept(); + await 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) { - device!.generator!.accept(); + await 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 daabad8b91..eefcf4ca3f 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) { - generator?.accept(); + await 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 c73e578df0..a8084cbb20 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) { - device.generator!.accept(); + await 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) { - device.generator!.accept(); + await 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 7bbadbb918..3181ef676b 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, ); - residentCompiler.accept(); + await 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 9c09ba92c4..e1ac7d8e5c 100644 --- a/packages/flutter_tools/lib/src/test/test_compiler.dart +++ b/packages/flutter_tools/lib/src/test/test_compiler.dart @@ -188,9 +188,11 @@ class TestCompiler { // compiler to avoid reusing compiler that might have gotten into // a weird state. if (outputPath == null || compilerOutput!.errorCount > 0) { - request.result.complete(); await _shutdown(); + request.result.complete(); } 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); @@ -209,8 +211,6 @@ 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 14749ae1d4..47c2a956fd 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 { - generator.accept(); + await 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 39f0aa0da5..c76f5397f1 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 - void accept() {} + Future accept() async {} } 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 165c00d234..3bab03eb44 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 - void accept() { } + Future accept() async {} @override - void reset() { } + Future reset() async {} } 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 563b0eac30..67858cad08 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 - void accept() {} + Future accept() async {} @override - void reset() {} + Future reset() async {} @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 257ad4f999..9585fbe924 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 - void accept() { } + Future accept() async {} @override - void reset() { } + Future reset() async {} @override Future shutdown() async {