Fix a race condition with re-used compilation isolate IDs (#2018)

Closes #2004

CHANGELOG.md

@@ -9,6 +9,9 @@
 
 * Fix a deadlock when running at high concurrency on 32-bit systems.
 
+* Fix a race condition where the embedded compiler could deadlock or crash if a
+  compilation ID was reused immediately after the compilation completed.
+
 ## 1.63.4
 
 ### JavaScript API

lib/src/embedded/dispatcher.dart

@@ -322,10 +322,17 @@ class Dispatcher {
     var protobufWriter = CodedBufferWriter();
     message.writeToCodedBufferWriter(protobufWriter);
 
-    var packet =
+    // Add one additional byte to the beginning to indicate whether or not the
-        Uint8List(_compilationIdVarint.length + protobufWriter.lengthInBytes);
+    // compilation is finished, so the [IsolateDispatcher] knows whether to
-    packet.setAll(0, _compilationIdVarint);
+    // treat this isolate as inactive.
-    protobufWriter.writeTo(packet, _compilationIdVarint.length);
+    var packet = Uint8List(
+        1 + _compilationIdVarint.length + protobufWriter.lengthInBytes);
+    packet[0] =
+        message.whichMessage() == OutboundMessage_Message.compileResponse
+            ? 1
+            : 0;
+    packet.setAll(1, _compilationIdVarint);
+    protobufWriter.writeTo(packet, 1 + _compilationIdVarint.length);
     _channel.sink.add(packet);
   }
 }

lib/src/embedded/isolate_dispatcher.dart

@@ -158,19 +158,27 @@ class IsolateDispatcher {
     var receivePort = ReceivePort();
     isolate.sink.add((receivePort.sendPort, compilationId));
 
-    var channel = IsolateChannel<Uint8List?>.connectReceive(receivePort)
+    var channel = IsolateChannel<Uint8List>.connectReceive(receivePort);
-        .transform(const ExplicitCloseTransformer());
+    channel.stream.listen(
-    channel.stream.listen(_channel.sink.add,
+        (message) {
+          // The first byte of messages from isolates indicates whether the
+          // entire compilation is finished. Sending this as part of the message
+          // buffer rather than a separate message avoids a race condition where
+          // the host might send a new compilation request with the same ID as
+          // one that just finished before the [IsolateDispatcher] receives word
+          // that the isolate with that ID is done. See sass/dart-sass#2004.
+          if (message[0] == 1) {
+            channel.sink.close();
+            _activeIsolates.remove(compilationId);
+            _inactiveIsolates.add(isolate);
+            resource.release();
+          }
+          _channel.sink.add(Uint8List.sublistView(message, 1));
+        },
         onError: (Object error, StackTrace stackTrace) =>
             _handleError(error, stackTrace),
         onDone: () {
-          _activeIsolates.remove(compilationId);
+          if (_closed) isolate.sink.close();
-          if (_closed) {
-            isolate.sink.close();
-          } else {
-            _inactiveIsolates.add(isolate);
-          }
-          resource.release();
         });
     _activeIsolates[compilationId] = channel.sink;
     return channel.sink;
@@ -228,8 +236,7 @@ void _isolateMain(SendPort sendPort) {
   channel.stream.listen((initialMessage) async {
     var (compilationSendPort, compilationId) = initialMessage;
     var compilationChannel =
-        IsolateChannel<Uint8List?>.connectSend(compilationSendPort)
+        IsolateChannel<Uint8List>.connectSend(compilationSendPort);
-            .transform(const ExplicitCloseTransformer());
     var success = await Dispatcher(compilationChannel, compilationId).listen();
     if (!success) channel.sink.close();
   });

pkg/sass_api/CHANGELOG.md

@@ -1,3 +1,7 @@
+## 7.1.5
+
+* No user-visible changes.
+
 ## 7.1.4
 
 * No user-visible changes.

pkg/sass_api/pubspec.yaml

@@ -2,7 +2,7 @@ name: sass_api
 # Note: Every time we add a new Sass AST node, we need to bump the *major*
 # version because it's a breaking change for anyone who's implementing the
 # visitor interface(s).
-version: 7.1.4
+version: 7.1.5
 description: Additional APIs for Dart Sass.
 homepage: https://github.com/sass/dart-sass
 
@@ -10,7 +10,7 @@ environment:
   sdk: ">=3.0.0 <4.0.0"
 
 dependencies:
-  sass: 1.63.4
+  sass: 1.63.5
 
 dev_dependencies:
   dartdoc: ^5.0.0

pubspec.yaml

@@ -1,5 +1,5 @@
 name: sass
-version: 1.63.5-dev
+version: 1.63.5
 description: A Sass implementation in Dart.
 homepage: https://github.com/sass/dart-sass
 

test/embedded/protocol_test.dart

@@ -224,6 +224,16 @@ void main() {
     await process.shouldExit(0);
   });
 
+  // Regression test for sass/dart-sass#2004
+  test("handles many sequential compilation requests", () async {
+    var totalRequests = 1000;
+    for (var i = 1; i <= totalRequests; i++) {
+      process.send(compileString("a {b: 1px + 2px}"));
+      await expectSuccess(process, "a { b: 3px; }");
+    }
+    await process.close();
+  });
+
   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