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 flutter/flutter#153026
Original PR Author: andrewkolos

Reviewed By: {christopherfujino}

This change reverts the following previous change:
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: auto-submit[bot]

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

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<void> 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<void> reset();
+  void reset();
 
   Future<Object> 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<Uri>? 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(<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(),
-    ]);
+    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(<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 ?? ''
-    ]);
+    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<void> 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<CompilerOutput?>.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<void> 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<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) {
-      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

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

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

packages/flutter_tools/lib/src/resident_runner.dart

@@ -595,7 +595,7 @@ class FlutterDevice {
 
   Future<void> updateReloadStatus(bool wasReloadSuccessful) async {
     if (wasReloadSuccessful) {
-      await generator?.accept();
+      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) {
-        await device.generator!.accept();
+        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) {
-        await device.generator!.accept();
+        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,
     );
-    await residentCompiler.accept();
+    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,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!);

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

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