Fix race condition between spawning and killing isolates during shutd…

…own (#2007)

Co-authored-by: Natalie Weizenbaum <nweiz@google.com>

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.
 

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

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