From 3684897782f757a38663fd690022cdf497a9f9bc Mon Sep 17 00:00:00 2001 From: Nate Biggs Date: Fri, 20 Dec 2024 16:27:53 -0500 Subject: [PATCH] [dwds] Add unbatching retry mechanism for failing batched expression evals (#2552) [dwds] Add unbatching retry mechanism for failing batched expression evals. --------- Co-authored-by: Nate Biggs --- dwds/CHANGELOG.md | 1 + .../batched_expression_evaluator.dart | 53 +++++++++---------- dwds/test/expression_evaluator_test.dart | 20 ++++++- dwds/test/fixtures/fakes.dart | 11 +++- 4 files changed, 54 insertions(+), 31 deletions(-) diff --git a/dwds/CHANGELOG.md b/dwds/CHANGELOG.md index 55eb577d2..766a42b7d 100644 --- a/dwds/CHANGELOG.md +++ b/dwds/CHANGELOG.md @@ -2,6 +2,7 @@ - Update to be forward compatible with changes to `package:shelf_web_socket`. - Added support for some debugging APIs with the DDC library bundle format. - [#2537](https://github.com/dart-lang/webdev/issues/2537),[#2544](https://github.com/dart-lang/webdev/issues/2544) +- Fix issue where batched expression evals were failing if any subexpression failed. - [#2551](https://github.com/dart-lang/webdev/issues/2551) ## 24.2.0 diff --git a/dwds/lib/src/services/batched_expression_evaluator.dart b/dwds/lib/src/services/batched_expression_evaluator.dart index 3214ba7af..cd3d1619e 100644 --- a/dwds/lib/src/services/batched_expression_evaluator.dart +++ b/dwds/lib/src/services/batched_expression_evaluator.dart @@ -134,38 +134,37 @@ class BatchedExpressionEvaluator extends ExpressionEvaluator { first.scope, ); + final listId = list.objectId; + if (listId == null) { + for (final request in requests) { + safeUnawaited(_evaluateBatch([request])); + } + return; + } + for (var i = 0; i < requests.length; i++) { final request = requests[i]; if (request.completer.isCompleted) continue; _logger.fine('Getting result out of a batch for ${request.expression}'); - final listId = list.objectId; - if (listId == null) { - final error = createError( - EvaluationErrorKind.internal, - 'No batch result object ID.', - ); - request.completer.complete(error); - } else { - safeUnawaited( - _inspector - .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); - }), - onError: (error, stackTrace) => - request.completer.completeError(error, stackTrace), - ); - } + safeUnawaited( + _inspector + .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); + }), + onError: (error, stackTrace) => + request.completer.completeError(error, stackTrace), + ); } } } diff --git a/dwds/test/expression_evaluator_test.dart b/dwds/test/expression_evaluator_test.dart index 4405a891d..63d54dee8 100644 --- a/dwds/test/expression_evaluator_test.dart +++ b/dwds/test/expression_evaluator_test.dart @@ -13,6 +13,7 @@ import 'package:dwds/src/debugging/location.dart'; import 'package:dwds/src/debugging/skip_list.dart'; import 'package:dwds/src/services/batched_expression_evaluator.dart'; import 'package:dwds/src/services/expression_evaluator.dart'; +import 'package:dwds/src/utilities/shared.dart'; import 'package:test/test.dart'; import 'package:vm_service/vm_service.dart' hide LogRecord; @@ -36,6 +37,7 @@ void main() async { late StreamController pausedController; late StreamController debugEventController; + late FakeInspector inspector; setUp(() async { final assetReader = FakeAssetReader(sourceMap: ''); final toolConfiguration = TestToolConfiguration.withLoadStrategy( @@ -64,8 +66,7 @@ void main() async { skipLists, root, ); - final inspector = - FakeInspector(webkitDebugger, fakeIsolate: simpleIsolate); + inspector = FakeInspector(webkitDebugger, fakeIsolate: simpleIsolate); debugger.updateInspector(inspector); _evaluator = ExpressionEvaluator( @@ -192,6 +193,21 @@ void main() async { ); }); + test('retries failed batched expression', () async { + safeUnawaited( + evaluator.evaluateExpression('2', 'main.dart', 'true', {}), + ); + + await evaluator.evaluateExpression('2', 'main.dart', 'false', {}); + expect(inspector.functionsCalled.length, 3); + expect( + inspector.functionsCalled[0].contains('return [ true, false ];'), + true, + ); + expect(inspector.functionsCalled[1].contains('return true;'), true); + expect(inspector.functionsCalled[2].contains('return false;'), true); + }); + test('returns error if closed', () async { evaluator.close(); final result = diff --git a/dwds/test/fixtures/fakes.dart b/dwds/test/fixtures/fakes.dart index 0d82722f3..18e74dc0b 100644 --- a/dwds/test/fixtures/fakes.dart +++ b/dwds/test/fixtures/fakes.dart @@ -48,6 +48,7 @@ Isolate get simpleIsolate => Isolate( class FakeInspector implements AppInspector { final WebkitDebugger _remoteDebugger; + final List functionsCalled = []; FakeInspector(this._remoteDebugger, {required this.fakeIsolate}); Isolate fakeIsolate; @@ -61,8 +62,10 @@ class FakeInspector implements AppInspector { Future callFunction( String function, Iterable argumentIds, - ) async => - RemoteObject({'type': 'string', 'value': 'true'}); + ) async { + functionsCalled.add(function); + return RemoteObject({'type': 'string', 'value': 'true'}); + } @override Future initialize() async => {}; @@ -473,3 +476,7 @@ final fakeWipResponse = WipResponse({ 'id': 1, 'result': {'fake': ''}, }); + +final fakeFailingWipResponse = WipResponse({ + 'result': 'Error: Bad request', +});