[VM/Service] Correctly forward errors returned by external clients th…

…at handle 'compileExpression' requests TEST=pkg/vm_service/test/forward_compile_expression_error_from_external_client_with_dds_test.dart and pkg/vm_service/test/forward_compile_expression_error_from_external_client_without_dds_test.dart CoreLibraryReviewExempt: This CL does not include any core library API changes, only VM Service implementation changes in sdk/lib/vmservice/running_isolates.dart. Fixes: #59603 Change-Id: I2b9edf69feb6149c80afe9fc753c73e069af0479 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/397580 Reviewed-by: Ben Konyi <[email protected]> Commit-Queue: Derek Xu <[email protected]>
dart-lang · Nov 27, 2024 · 3f6928c · 3f6928c
1 parent e3ad3b5
commit 3f6928c
Show file tree

Hide file tree

Showing 5 changed files with 126 additions and 20 deletions.
diff --git a/pkg/pkg.status b/pkg/pkg.status
@@ -138,6 +138,8 @@ vm_service/test/eval_*test: SkipByDesign # Debugger is disabled in AOT mode.
 vm_service/test/evaluate_*test: SkipByDesign # Debugger is disabled in AOT mode.
 vm_service/test/external_compilation_service_test: SkipByDesign # Spawns a secondary process.
 vm_service/test/field_script_test: SkipByDesign # Debugger is disabled in AOT mode.
+vm_service/test/forward_compile_expression_error_from_external_client_with_dds_test: SkipByDesign # Debugger is disabled in AOT mode.
+vm_service/test/forward_compile_expression_error_from_external_client_without_dds_test: SkipByDesign # Debugger is disabled in AOT mode.
 vm_service/test/get_allocation_traces_test: SkipByDesign # Debugger is disabled in AOT mode.
 vm_service/test/get_instances_as_*: SkipByDesign # Debugger is disabled in AOT mode
 vm_service/test/get_instances_rpc_test: SkipByDesign # Debugger is disabled in AOT mode.

diff --git a/pkg/vm_service/test/forward_compile_expression_error_from_external_client_test_common.dart b/pkg/vm_service/test/forward_compile_expression_error_from_external_client_test_common.dart
@@ -0,0 +1,47 @@
+// Copyright (c) 2024, 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.
+
+import 'package:test/test.dart';
+import 'package:vm_service/vm_service.dart';
+import 'package:vm_service/vm_service_io.dart' show vmServiceConnectUri;
+
+import 'common/service_test_common.dart';
+
+final tests = <IsolateTest>[
+  hasStoppedAtExit,
+  (VmService primaryClient, IsolateRef isolateRef) async {
+    const expressionCompilationFailedMessage = 'Expresion compilation failed';
+
+    final secondaryClient = await vmServiceConnectUri(primaryClient.wsUri!);
+    secondaryClient.registerServiceCallback('compileExpression',
+        (params) async {
+      return {
+        'jsonrpc': '2.0',
+        'id': 0,
+        'error': {
+          'code': RPCErrorKind.kExpressionCompilationError.code,
+          'message': expressionCompilationFailedMessage,
+          'data': {'details': expressionCompilationFailedMessage},
+        },
+      };
+    });
+    await secondaryClient.registerService(
+      'compileExpression',
+      'Custom Expression Compilation',
+    );
+
+    final isolateId = isolateRef.id!;
+    try {
+      final isolate = await primaryClient.getIsolate(isolateId);
+      await primaryClient.evaluate(isolateId, isolate.rootLib!.id!, '123');
+      fail('Expected to catch an RPCError');
+    } on RPCError catch (e) {
+      expect(e.code, RPCErrorKind.kExpressionCompilationError.code);
+      // [e.details] used to be the string
+      // "{code: 113, message: Expresion compilation failed, data: ...}", so we
+      // want to avoid regressing to that behaviour.
+      expect(e.details, expressionCompilationFailedMessage);
+    }
+  },
+];
diff --git a/pkg/vm_service/test/forward_compile_expression_error_from_external_client_with_dds_test.dart b/pkg/vm_service/test/forward_compile_expression_error_from_external_client_with_dds_test.dart
@@ -0,0 +1,15 @@
+// Copyright (c) 2024, 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.
+
+// This is a regression test for https://dartbug.com/59603.
+
+import 'common/test_helper.dart';
+import 'forward_compile_expression_error_from_external_client_test_common.dart';
+
+void main([args = const <String>[]]) => runIsolateTests(
+      args,
+      tests,
+      'forward_compile_expression_error_from_external_client_with_dds_test.dart',
+      pauseOnExit: true,
+    );
diff --git a/..._service/test/forward_compile_expression_error_from_external_client_without_dds_test.dart b/..._service/test/forward_compile_expression_error_from_external_client_without_dds_test.dart
@@ -0,0 +1,16 @@
+// Copyright (c) 2024, 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.
+
+// This is a regression test for https://dartbug.com/59603.
+
+import 'common/test_helper.dart';
+import 'forward_compile_expression_error_from_external_client_test_common.dart';
+
+void main([args = const <String>[]]) => runIsolateTests(
+      args,
+      tests,
+      'forward_compile_expression_error_from_external_client_without_dds_test.dart',
+      pauseOnExit: true,
+      extraArgs: ['--no-dds'],
+    );
diff --git a/sdk/lib/vmservice/running_isolates.dart b/sdk/lib/vmservice/running_isolates.dart
@@ -4,6 +4,12 @@
 
 part of dart._vmservice;
 
+final class _CompileExpressionErrorDetails {
+  final String details;
+
+  _CompileExpressionErrorDetails(this.details);
+}
+
 class RunningIsolates implements MessageRouter {
   final isolates = <int, RunningIsolate>{};
   int? _rootPortId;
@@ -96,8 +102,14 @@ class _Evaluator {
       kernelBase64 = await _compileExpression(
         responseJson['result'] as Map<String, dynamic>,
       );
-    } catch (e) {
-      return Response.from(encodeCompilationError(_message, e.toString()));
+    } on _CompileExpressionErrorDetails catch (e) {
+      return Response.from(
+        encodeRpcError(
+          _message,
+          kExpressionCompilationError,
+          details: e.details,
+        ),
+      );
     }
     return await _evaluateCompiledExpression(kernelBase64);
   }
@@ -128,6 +140,26 @@ class _Evaluator {
     return _isolate.routeRequest(_service, buildScope);
   }
 
+  /// If [response] represents a valid JSON-RPC result, then this function
+  /// returns the 'kernelBytes' property of that result. Otherwise, this
+  /// function throws a [_CompileExpressionErrorDetails] object wrapping the
+  /// 'details' property of the JSON-RPC error.
+  static String _getKernelBytesOrThrowErrorDetails(
+    Map<String, dynamic> response,
+  ) {
+    if (response['result'] != null) {
+      return (response['result'] as Map<String, dynamic>)['kernelBytes']
+          as String;
+    }
+    final error = response['error'] as Map<String, dynamic>;
+    final data = error['data'] as Map<String, dynamic>;
+    throw _CompileExpressionErrorDetails(data['details']);
+  }
+
+  /// If compilation fails, this method will throw a
+  /// [_CompileExpressionErrorDetails] object that will be used to populate the
+  /// 'details' field of the response to the evaluation RPC that requested this
+  /// compilation to happen.
   Future<String> _compileExpression(
     Map<String, dynamic> buildScopeResponseResult,
   ) {
@@ -182,14 +214,13 @@ class _Evaluator {
           }),
         ),
       );
-      return completer.future.then((s) => jsonDecode(s)).then((json) {
-        final jsonMap = json as Map<String, dynamic>;
-        if (jsonMap.containsKey('error')) {
-          throw jsonMap['error'] as Object;
-        }
-        return (jsonMap['result'] as Map<String, dynamic>)['kernelBytes']
-            as String;
-      });
+      return completer.future
+          .then((s) => jsonDecode(s))
+          .then(
+            (json) => _getKernelBytesOrThrowErrorDetails(
+              json as Map<String, dynamic>,
+            ),
+          );
     } else {
       // fallback to compile using kernel service
       final compileExpressionParams = <String, dynamic>{
@@ -205,16 +236,11 @@ class _Evaluator {
       return _isolate
           .routeRequest(_service, compileExpression)
           .then((response) => response.decodeJson())
-          .then((json) {
-            final response = json as Map<String, dynamic>;
-            if (response['result'] != null) {
-              return (response['result'] as Map<String, dynamic>)['kernelBytes']
-                  as String;
-            }
-            final error = response['error'] as Map<String, dynamic>;
-            final data = error['data'] as Map<String, dynamic>;
-            throw data['details'] as Object;
-          });
+          .then(
+            (json) => _getKernelBytesOrThrowErrorDetails(
+              json as Map<String, dynamic>,
+            ),
+          );
     }
   }