From aa59a5f4a91f7dafb15c9b88b0970de7aca19db1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=E3=81=AA=E3=81=A4=E3=81=8D?= <i@ntk.me>
Date: Fri, 9 Jun 2023 15:55:47 -0700
Subject: [PATCH] Fix race condition between spawning and killing isolates
 during shutdown (#2007)

Co-authored-by: Natalie Weizenbaum <nweiz@google.com>
---
 CHANGELOG.md                             |  4 ++++
 lib/src/embedded/isolate_dispatcher.dart | 10 ++++++----
 test/embedded/protocol_test.dart         | 13 +++++++++++++
 3 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 516a8d53e..747deb5d2 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,6 +2,10 @@
 
 ### Embedded Sass
 
+* Fix a race condition where closing standard input while requests are in-flight
+  could sometimes cause the process to hang rather than shutting down
+  gracefully.
+
 * Properly include the root stylesheet's URL in the set of loaded URLs when it
   fails to parse.
 
diff --git a/lib/src/embedded/isolate_dispatcher.dart b/lib/src/embedded/isolate_dispatcher.dart
index 6951a77f5..71c4b041a 100644
--- a/lib/src/embedded/isolate_dispatcher.dart
+++ b/lib/src/embedded/isolate_dispatcher.dart
@@ -46,7 +46,7 @@ class IsolateDispatcher {
   /// The actual isolate objects that have been spawned.
   ///
   /// Only used for cleaning up the process when the underlying channel closes.
-  final _allIsolates = <Isolate>[];
+  final _allIsolates = <Future<Isolate>>[];
 
   /// A pool controlling how many isolates (and thus concurrent compilations)
   /// may be live at once.
@@ -101,10 +101,10 @@ class IsolateDispatcher {
       }
     }, onError: (Object error, StackTrace stackTrace) {
       _handleError(error, stackTrace);
-    }, onDone: () {
+    }, onDone: () async {
       _closed = true;
       for (var isolate in _allIsolates) {
-        isolate.kill();
+        (await isolate).kill();
       }
 
       // Killing isolates isn't sufficient to make sure the process closes; we
@@ -130,7 +130,9 @@ class IsolateDispatcher {
     }
 
     var receivePort = ReceivePort();
-    _allIsolates.add(await Isolate.spawn(_isolateMain, receivePort.sendPort));
+    var future = Isolate.spawn(_isolateMain, receivePort.sendPort);
+    _allIsolates.add(future);
+    await future;
 
     var channel = IsolateChannel<_InitialMessage?>.connectReceive(receivePort)
         .transform(const ExplicitCloseTransformer());
diff --git a/test/embedded/protocol_test.dart b/test/embedded/protocol_test.dart
index 663c28a5e..d302a9753 100644
--- a/test/embedded/protocol_test.dart
+++ b/test/embedded/protocol_test.dart
@@ -224,6 +224,19 @@ void main() {
     await process.shouldExit(0);
   });
 
+  test("closes gracefully with many in-flight compilations", () async {
+    // This should always be equal to the size of
+    // [IsolateDispatcher._isolatePool], since that's as many concurrent
+    // compilations as we can realistically have anyway.
+    var totalRequests = 15;
+    for (var i = 1; i <= totalRequests; i++) {
+      process.inbound
+          .add((i, compileString("a {b: foo() + 2px}", functions: [r"foo()"])));
+    }
+
+    await process.close();
+  }, skip: "Enable once dart-lang/stream_channel#92 is released");
+
   test("doesn't include a source map by default", () async {
     process.send(compileString("a {b: 1px + 2px}"));
     await expectSuccess(process, "a { b: 3px; }", sourceMap: isEmpty);