From a48ced8ec9c60a61af6a2fc280f909e6f05627cf Mon Sep 17 00:00:00 2001 From: Natalie Weizenbaum Date: Tue, 20 Jun 2023 17:55:46 -0700 Subject: [PATCH] Fix a race condition with re-used compilation isolate IDs (#2018) Closes #2004 --- CHANGELOG.md | 3 +++ lib/src/embedded/dispatcher.dart | 15 +++++++++--- lib/src/embedded/isolate_dispatcher.dart | 31 +++++++++++++++--------- pkg/sass_api/CHANGELOG.md | 4 +++ pkg/sass_api/pubspec.yaml | 4 +-- pubspec.yaml | 2 +- test/embedded/protocol_test.dart | 10 ++++++++ 7 files changed, 50 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5a3c6f3a3..dd4a720e3 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/lib/src/embedded/dispatcher.dart b/lib/src/embedded/dispatcher.dart index 0e9c6899f..514552fc0 100644 --- a/lib/src/embedded/dispatcher.dart +++ b/lib/src/embedded/dispatcher.dart @@ -322,10 +322,17 @@ class Dispatcher { var protobufWriter = CodedBufferWriter(); message.writeToCodedBufferWriter(protobufWriter); - var packet = - Uint8List(_compilationIdVarint.length + protobufWriter.lengthInBytes); - packet.setAll(0, _compilationIdVarint); - protobufWriter.writeTo(packet, _compilationIdVarint.length); + // Add one additional byte to the beginning to indicate whether or not the + // compilation is finished, so the [IsolateDispatcher] knows whether to + // treat this isolate as inactive. + 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); } } diff --git a/lib/src/embedded/isolate_dispatcher.dart b/lib/src/embedded/isolate_dispatcher.dart index 0fac523ed..754bfcbce 100644 --- a/lib/src/embedded/isolate_dispatcher.dart +++ b/lib/src/embedded/isolate_dispatcher.dart @@ -158,19 +158,27 @@ class IsolateDispatcher { var receivePort = ReceivePort(); isolate.sink.add((receivePort.sendPort, compilationId)); - var channel = IsolateChannel.connectReceive(receivePort) - .transform(const ExplicitCloseTransformer()); - channel.stream.listen(_channel.sink.add, + var channel = IsolateChannel.connectReceive(receivePort); + channel.stream.listen( + (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(); - } else { - _inactiveIsolates.add(isolate); - } - resource.release(); + if (_closed) isolate.sink.close(); }); _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.connectSend(compilationSendPort) - .transform(const ExplicitCloseTransformer()); + IsolateChannel.connectSend(compilationSendPort); var success = await Dispatcher(compilationChannel, compilationId).listen(); if (!success) channel.sink.close(); }); diff --git a/pkg/sass_api/CHANGELOG.md b/pkg/sass_api/CHANGELOG.md index 2addc1cf0..eb1cdb918 100644 --- a/pkg/sass_api/CHANGELOG.md +++ b/pkg/sass_api/CHANGELOG.md @@ -1,3 +1,7 @@ +## 7.1.5 + +* No user-visible changes. + ## 7.1.4 * No user-visible changes. diff --git a/pkg/sass_api/pubspec.yaml b/pkg/sass_api/pubspec.yaml index 6df55aeb1..d34766a14 100644 --- a/pkg/sass_api/pubspec.yaml +++ b/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 diff --git a/pubspec.yaml b/pubspec.yaml index 52eb8a43c..cbbfd5f90 100644 --- a/pubspec.yaml +++ b/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 diff --git a/test/embedded/protocol_test.dart b/test/embedded/protocol_test.dart index d302a9753..ff885d1b5 100644 --- a/test/embedded/protocol_test.dart +++ b/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