Return error from expression evaluation if the evaluator is closed.

dwds/CHANGELOG.md

@@ -16,6 +16,7 @@
   - Do not pass `--(no)-sound-null-safety` flag to build daemon.
 - Add back `ChromeProxyService.setExceptionPauseMode()` without override.
 - Make hot restart atomic to prevent races on simultaneous execution.
+- Return error on expression evaluation if expression evaluator stopped.
 
 **Breaking changes**
 - Include an optional param to `Dwds.start` to indicate whether it is running

dwds/lib/src/services/batched_expression_evaluator.dart

@@ -31,6 +31,7 @@ class BatchedExpressionEvaluator extends ExpressionEvaluator {
   final Debugger _debugger;
   final _requestController =
       BatchedStreamController<EvaluateRequest>(delay: 200);
+  bool _closed = false;
 
   BatchedExpressionEvaluator(
     String entrypoint,
@@ -45,8 +46,10 @@ class BatchedExpressionEvaluator extends ExpressionEvaluator {
 
   @override
   void close() {
+    if (_closed) return;
     _logger.fine('Closed');
     _requestController.close();
+    _closed = true;
   }
 
   @override
@@ -55,7 +58,11 @@ class BatchedExpressionEvaluator extends ExpressionEvaluator {
     String? libraryUri,
     String expression,
     Map<String, String>? scope,
-  ) {
+  ) async {
+    if (_closed) {
+      return createError(
+          ErrorKind.internal, 'Batched expression evaluator closed');
+    }
     final request = EvaluateRequest(isolateId, libraryUri, expression, scope);
     _requestController.sink.add(request);
     return request.completer.future;
@@ -121,15 +128,22 @@ class BatchedExpressionEvaluator extends ExpressionEvaluator {
       final request = requests[i];
       if (request.completer.isCompleted) continue;
       _logger.fine('Getting result out of a batch for ${request.expression}');
-      unawaited(_debugger
-          .getProperties(list.objectId!,
-              offset: i, count: 1, length: requests.length)
-          .then((v) {
-        final result = v.first.value;
-        _logger.fine(
-            'Got result out of a batch for ${request.expression}: $result');
-        request.completer.complete(result);
-      }));
+
+      final listId = list.objectId;
+      if (listId == null) {
+        final error =
+            createError(ErrorKind.internal, 'No batch result object ID.');
+        request.completer.complete(error);
+      } else {
+        unawaited(_debugger
+            .getProperties(listId, offset: i, count: 1, length: requests.length)
+            .then((v) {
+          final result = v.first.value;
+          _logger.fine(
+              'Got result out of a batch for ${request.expression}: $result');
+          request.completer.complete(result);
+        }));
+      }
     }
   }
 }

dwds/lib/src/services/chrome_proxy_service.dart

@@ -444,10 +444,20 @@ ${globalLoadStrategy.loadModuleSnippet}("dart_sdk").developer.invokeExtension(
     return _rpcNotSupportedFuture('clearVMTimeline');
   }
 
-  Future<Response> _getEvaluationResult(
+  Future<Response> _getEvaluationResult(String isolateId,
       Future<RemoteObject> Function() evaluation, String expression) async {
     try {
       final result = await evaluation();
+      if (!_isIsolateRunning || isolateId != inspector.isolate.id) {
+        _logger.fine('Cannot get evaluation result for isolate $isolateId: '
+            ' isolate exited.');
+        return ErrorRef(
+          kind: 'error',
+          message: 'Isolate exited',
+          id: createId(),
+        );
+      }
+
       // Handle compilation errors, internal errors,
       // and reference errors from JavaScript evaluation in chrome.
       if (result.type.contains('Error')) {
@@ -498,6 +508,7 @@ ${globalLoadStrategy.loadModuleSnippet}("dart_sdk").developer.invokeExtension(
 
         final library = await inspector.getLibrary(targetId);
         return await _getEvaluationResult(
+            isolateId,
             () => evaluator.evaluateExpression(
                 isolateId, library?.uri, expression, scope),
             expression);
@@ -521,6 +532,7 @@ ${globalLoadStrategy.loadModuleSnippet}("dart_sdk").developer.invokeExtension(
         _checkIsolate('evaluateInFrame', isolateId);
 
         return await _getEvaluationResult(
+            isolateId,
             () => evaluator.evaluateExpressionInFrame(
                 isolateId, frameIndex, expression, scope),
             expression);

dwds/lib/src/services/expression_evaluator.dart

@@ -41,6 +41,7 @@ class ExpressionEvaluator {
   final Modules _modules;
   final ExpressionCompiler _compiler;
   final _logger = Logger('ExpressionEvaluator');
+  bool _closed = false;
 
   /// Strip synthetic library name from compiler error messages.
   static final _syntheticNameFilterRegex =
@@ -56,12 +57,14 @@ class ExpressionEvaluator {
   ExpressionEvaluator(this._entrypoint, this._inspector, this._debugger,
       this._locations, this._modules, this._compiler);
 
-  RemoteObject _createError(ErrorKind severity, String message) {
+  RemoteObject createError(ErrorKind severity, String message) {
     return RemoteObject(
         <String, String>{'type': '$severity', 'value': message});
   }
 
-  void close() {}
+  void close() {
+    _closed = true;
+  }
 
   /// Evaluate dart expression inside a given library.
   ///
@@ -80,19 +83,23 @@ class ExpressionEvaluator {
     String expression,
     Map<String, String>? scope,
   ) async {
+    if (_closed) {
+      return createError(ErrorKind.internal, 'expression evaluator closed.');
+    }
+
     scope ??= {};
 
     if (expression.isEmpty) {
-      return _createError(ErrorKind.invalidInput, expression);
+      return createError(ErrorKind.invalidInput, expression);
     }
 
     if (libraryUri == null) {
-      return _createError(ErrorKind.invalidInput, 'no library uri');
+      return createError(ErrorKind.invalidInput, 'no library uri');
     }
 
     final module = await _modules.moduleForLibrary(libraryUri);
     if (module == null) {
-      return _createError(ErrorKind.internal, 'no module for $libraryUri');
+      return createError(ErrorKind.internal, 'no module for $libraryUri');
     }
 
     // Wrap the expression in a lambda so we can call it as a function.
@@ -118,7 +125,8 @@ class ExpressionEvaluator {
     var result = await _inspector.callFunction(jsCode, scope.values);
     result = await _formatEvaluationError(result);
 
-    _logger.finest('Evaluated "$expression" to "$result"');
+    _logger
+        .finest('Evaluated "$expression" to "$result" for isolate $isolateId');
     return result;
   }
 
@@ -139,20 +147,20 @@ class ExpressionEvaluator {
     if (scope != null) {
       // TODO(annagrin): Implement scope support.
       // Issue: https://github.com/dart-lang/webdev/issues/1344
-      return _createError(
+      return createError(
           ErrorKind.internal,
           'Using scope for expression evaluation in frame '
           'is not supported.');
     }
 
     if (expression.isEmpty) {
-      return _createError(ErrorKind.invalidInput, expression);
+      return createError(ErrorKind.invalidInput, expression);
     }
 
     // Get JS scope and current JS location.
     final jsFrame = _debugger.jsFrameForIndex(frameIndex);
     if (jsFrame == null) {
-      return _createError(
+      return createError(
           ErrorKind.internal,
           'Expression evaluation in async frames '
           'is not supported. No frame with index $frameIndex.');
@@ -167,12 +175,12 @@ class ExpressionEvaluator {
     // Find corresponding dart location and scope.
     final url = _debugger.urlForScriptId(jsScriptId);
     if (url == null) {
-      return _createError(
+      return createError(
           ErrorKind.internal, 'Cannot find url for JS script: $jsScriptId');
     }
     final locationMap = await _locations.locationForJs(url, jsLine, jsColumn);
     if (locationMap == null) {
-      return _createError(
+      return createError(
           ErrorKind.internal,
           'Cannot find Dart location for JS location: '
           'url: $url, '
@@ -185,13 +193,13 @@ class ExpressionEvaluator {
     final dartSourcePath = dartLocation.uri.serverPath;
     final libraryUri = await _modules.libraryForSource(dartSourcePath);
     if (libraryUri == null) {
-      return _createError(
+      return createError(
           ErrorKind.internal, 'no libraryUri for $dartSourcePath');
     }
 
     final module = await _modules.moduleForLibrary(libraryUri.toString());
     if (module == null) {
-      return _createError(
+      return createError(
           ErrorKind.internal, 'no module for $libraryUri ($dartSourcePath)');
     }
 
@@ -246,21 +254,21 @@ class ExpressionEvaluator {
     }
     if (error.contains('InternalError: ')) {
       error = error.replaceAll('InternalError: ', '');
-      return _createError(ErrorKind.internal, error);
+      return createError(ErrorKind.internal, error);
     }
     error = error.replaceAll(_syntheticNameFilterRegex, '');
-    return _createError(ErrorKind.compilation, error);
+    return createError(ErrorKind.compilation, error);
   }
 
   Future<RemoteObject> _formatEvaluationError(RemoteObject result) async {
     if (result.type == 'string') {
       var error = '${result.value}';
       if (error.startsWith('ReferenceError: ')) {
         error = error.replaceFirst('ReferenceError: ', '');
-        return _createError(ErrorKind.reference, error);
+        return createError(ErrorKind.reference, error);
       } else if (error.startsWith('TypeError: ')) {
         error = error.replaceFirst('TypeError: ', '');
-        return _createError(ErrorKind.type, error);
+        return createError(ErrorKind.type, error);
       } else if (error.startsWith('NetworkError: ')) {
         var modulePath = _loadModuleErrorRegex.firstMatch(error)?.group(1);
         final module = modulePath != null
@@ -271,7 +279,7 @@ class ExpressionEvaluator {
         error = 'Module is not loaded : $module (path: $modulePath). '
             'Accessing libraries that have not yet been used in the '
             'application is not supported during expression evaluation.';
-        return _createError(ErrorKind.loadModule, error);
+        return createError(ErrorKind.loadModule, error);
       }
     }
     return result;

dwds/test/expression_evaluator_test.dart

@@ -0,0 +1,137 @@
+// Copyright (c) 2020, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+@TestOn('vm')
+@Timeout(Duration(minutes: 2))
+import 'dart:async';
+
+import 'package:dwds/src/debugging/debugger.dart';
+import 'package:dwds/src/debugging/location.dart';
+import 'package:dwds/src/debugging/skip_list.dart';
+import 'package:dwds/src/loaders/strategy.dart';
+import 'package:dwds/src/services/batched_expression_evaluator.dart';
+import 'package:dwds/src/services/expression_evaluator.dart';
+
+import 'package:test/test.dart';
+import 'package:webkit_inspection_protocol/webkit_inspection_protocol.dart';
+
+import 'fixtures/fakes.dart';
+
+late ExpressionEvaluator? _evaluator;
+late ExpressionEvaluator? _batchedEvaluator;
+
+void main() async {
+  group('expression compiler service with fake asset server', () {
+    Future<void> stop() async {
+      _evaluator?.close();
+      _evaluator = null;
+      _batchedEvaluator?.close();
+      _batchedEvaluator = null;
+    }
+
+    setUp(() async {
+      globalLoadStrategy = FakeStrategy();
+
+      final assetReader = FakeAssetReader(sourceMap: '');
+      final modules = FakeModules();
+
+      final webkitDebugger = FakeWebkitDebugger(scripts: {});
+      final pausedController = StreamController<DebuggerPausedEvent>();
+      webkitDebugger.onPaused = pausedController.stream;
+
+      final root = 'fakeRoot';
+      final entry = 'fake_entrypoint';
+      final locations = Locations(assetReader, modules, root);
+      locations.initialize(entry);
+
+      final skipLists = SkipLists();
+      final debugger = await Debugger.create(
+        webkitDebugger,
+        (_, __) {},
+        locations,
+        skipLists,
+        root,
+      );
+      final inspector = FakeInspector(fakeIsolate: simpleIsolate);
+      debugger.updateInspector(inspector);
+
+      _evaluator = ExpressionEvaluator(
+        entry,
+        inspector,
+        debugger,
+        locations,
+        modules,
+        FakeExpressionCompiler(),
+      );
+      _batchedEvaluator = BatchedExpressionEvaluator(
+        entry,
+        inspector,
+        debugger,
+        locations,
+        modules,
+        FakeExpressionCompiler(),
+      );
+    });
+
+    tearDown(() async {
+      await stop();
+    });
+
+    group('evaluator', () {
+      late ExpressionEvaluator evaluator;
+
+      setUp(() async {
+        evaluator = _evaluator!;
+      });
+
+      test('can evaluate expression', () async {
+        final result =
+            await evaluator.evaluateExpression('1', 'main.dart', 'true', {});
+        expect(
+            result,
+            const TypeMatcher<RemoteObject>()
+                .having((o) => o.value, 'value', 'true'));
+      });
+
+      test('returns error if closed', () async {
+        evaluator.close();
+        final result =
+            await evaluator.evaluateExpression('1', 'main.dart', 'true', {});
+        expect(
+            result,
+            const TypeMatcher<RemoteObject>()
+                .having((o) => o.type, 'type', 'InternalError')
+                .having((o) => o.value, 'value', contains('evaluator closed')));
+      });
+    });
+
+    group('batched evaluator', () {
+      late ExpressionEvaluator evaluator;
+
+      setUp(() async {
+        evaluator = _batchedEvaluator!;
+      });
+
+      test('can evaluate expression', () async {
+        final result =
+            await evaluator.evaluateExpression('1', 'main.dart', 'true', {});
+        expect(
+            result,
+            const TypeMatcher<RemoteObject>()
+                .having((o) => o.value, 'value', 'true'));
+      });
+
+      test('returns error if closed', () async {
+        evaluator.close();
+        final result =
+            await evaluator.evaluateExpression('1', 'main.dart', 'true', {});
+        expect(
+            result,
+            const TypeMatcher<RemoteObject>()
+                .having((o) => o.type, 'type', 'InternalError')
+                .having((o) => o.value, 'value', contains('evaluator closed')));
+      });
+    });
+  });
+}