[tool] Guard process writes to frontend server in ResidentCompiler (#152358)

Contributes to fixing flutter/flutter#137184.
Cleaned up version of earlier PR, flutter/flutter#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.

Author: andrewkolos

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<void> 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<void> writeToStdinUnsafe({
     required IOSink stdin,
     required String content,

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<void> 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<void> reset();
 
   Future<Object> 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<Uri>? 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(<String>[
+      '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(<String>[
+      'compile-expression-to-js $inputKey',
+      request.libraryUri ?? '',
+      request.line.toString(),
+      request.column.toString(),
+      for (final MapEntry<String, String> entry in request.jsModules?.entries ?? <MapEntry<String, String>>[])
+        '${entry.key}:${entry.value}',
+      inputKey,
+      for (final MapEntry<String, String> entry in request.jsFrameValues?.entries ?? <MapEntry<String, String>>[])
+        '${entry.key}:${entry.value}',
+      inputKey,
+      request.moduleName ?? '',
+      request.expression ?? ''
+    ]);
 
     return _stdoutHandler.compilerOutput?.future;
   }
 
   @override
-  void accept() {
+  Future<void> 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<CompilerOutput?>.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<void> reset() async {
+    await _writelnToServerStdin('reset', printTrace: true);
   }
 
   @override
@@ -1064,6 +1060,43 @@ class DefaultResidentCompiler implements ResidentCompiler {
     server.kill();
     return server.exitCode;
   }
+
+  Future<void> _writelnToServerStdin(String line, {
+    bool printTrace = false,
+  }) async {
+    await _writelnToServerStdinAll(<String>[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<void> _writelnToServerStdinAll(List<String> 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

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

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

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();

packages/flutter_tools/lib/src/resident_runner.dart

@@ -595,7 +595,7 @@ class FlutterDevice {
 
   Future<void> updateReloadStatus(bool wasReloadSuccessful) async {
     if (wasReloadSuccessful) {
-      generator?.accept();
+      await generator?.accept();
     } else {
       await generator?.reject();
     }

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<FlutterView> 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.

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!);

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!);

packages/flutter_tools/test/general.shard/compile_incremental_test.dart

@@ -521,7 +521,7 @@ Future<void> _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));