Skip to content

Commit be616cd

Browse files
author
Anna Gringauze
authored
Return error from expression evaluation if the evaluator is closed. (#1884)
* Wait for isolate to be destroyed before creating a new one * Update changelog * Fix race condition on simultaneous hot restarts - Fix race condition on running two `createIsolate` calls simultaneously. - Run `destroyIsolate` followed by `createIsolate` an atomic operation. - Make debugger API that depend on isolate running wait for the start of the app. - Make debugger API throw if the isolate exits while the API is waiting. - Fix exception on uninitialized `DwdsStats._devToolsStart` when using an observatory uri to connect to the dwds VM service. - Remove unnecessary async keywords from synchronoous debugger API. Closes: ihttps://github.com//issues/1869 * Fix race condition on simultaneous hot restarts - Fix race condition on running two `createIsolate` calls simultaneously. - Run `destroyIsolate` followed by `createIsolate` an atomic operation. - Make debugger API that depend on isolate running wait for the start of the app. - Make debugger API throw if the isolate exits while the API is waiting. - Fix exception on uninitialized `DwdsStats._devToolsStart` when using an observatory uri to connect to the dwds VM service. - Remove unnecessary async keywords from synchronoous debugger API. Closes: ihttps://github.com//issues/1869 * Keep race condition changes only * Revert unneded changes again * Merged with master * Update changelog, pubspec, and build * Error on evaluation if expression evaluator is closed * Fix test failures * Remove unnecessary function parsin in FakeInspector
1 parent 18b3277 commit be616cd

File tree

7 files changed

+247
-66
lines changed

7 files changed

+247
-66
lines changed

‎dwds/CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
- Do not pass `--(no)-sound-null-safety` flag to build daemon.
1717
- Add back `ChromeProxyService.setExceptionPauseMode()` without override.
1818
- Make hot restart atomic to prevent races on simultaneous execution.
19+
- Return error on expression evaluation if expression evaluator stopped.
1920

2021
**Breaking changes**
2122
- Include an optional param to `Dwds.start` to indicate whether it is running

‎dwds/lib/src/services/batched_expression_evaluator.dart‎

Lines changed: 24 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ class BatchedExpressionEvaluator extends ExpressionEvaluator {
3131
final Debugger _debugger;
3232
final _requestController =
3333
BatchedStreamController<EvaluateRequest>(delay: 200);
34+
bool _closed = false;
3435

3536
BatchedExpressionEvaluator(
3637
String entrypoint,
@@ -45,8 +46,10 @@ class BatchedExpressionEvaluator extends ExpressionEvaluator {
4546

4647
@override
4748
void close() {
49+
if (_closed) return;
4850
_logger.fine('Closed');
4951
_requestController.close();
52+
_closed = true;
5053
}
5154

5255
@override
@@ -55,7 +58,11 @@ class BatchedExpressionEvaluator extends ExpressionEvaluator {
5558
String? libraryUri,
5659
String expression,
5760
Map<String, String>? scope,
58-
) {
61+
) async {
62+
if (_closed) {
63+
return createError(
64+
ErrorKind.internal, 'Batched expression evaluator closed');
65+
}
5966
final request = EvaluateRequest(isolateId, libraryUri, expression, scope);
6067
_requestController.sink.add(request);
6168
return request.completer.future;
@@ -121,15 +128,22 @@ class BatchedExpressionEvaluator extends ExpressionEvaluator {
121128
final request = requests[i];
122129
if (request.completer.isCompleted) continue;
123130
_logger.fine('Getting result out of a batch for ${request.expression}');
124-
unawaited(_debugger
125-
.getProperties(list.objectId!,
126-
offset: i, count: 1, length: requests.length)
127-
.then((v) {
128-
final result = v.first.value;
129-
_logger.fine(
130-
'Got result out of a batch for ${request.expression}: $result');
131-
request.completer.complete(result);
132-
}));
131+
132+
final listId = list.objectId;
133+
if (listId == null) {
134+
final error =
135+
createError(ErrorKind.internal, 'No batch result object ID.');
136+
request.completer.complete(error);
137+
} else {
138+
unawaited(_debugger
139+
.getProperties(listId, offset: i, count: 1, length: requests.length)
140+
.then((v) {
141+
final result = v.first.value;
142+
_logger.fine(
143+
'Got result out of a batch for ${request.expression}: $result');
144+
request.completer.complete(result);
145+
}));
146+
}
133147
}
134148
}
135149
}

‎dwds/lib/src/services/chrome_proxy_service.dart‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -444,10 +444,20 @@ ${globalLoadStrategy.loadModuleSnippet}("dart_sdk").developer.invokeExtension(
444444
return _rpcNotSupportedFuture('clearVMTimeline');
445445
}
446446

447-
Future<Response> _getEvaluationResult(
447+
Future<Response> _getEvaluationResult(String isolateId,
448448
Future<RemoteObject> Function() evaluation, String expression) async {
449449
try {
450450
final result = await evaluation();
451+
if (!_isIsolateRunning || isolateId != inspector.isolate.id) {
452+
_logger.fine('Cannot get evaluation result for isolate $isolateId: '
453+
' isolate exited.');
454+
return ErrorRef(
455+
kind: 'error',
456+
message: 'Isolate exited',
457+
id: createId(),
458+
);
459+
}
460+
451461
// Handle compilation errors, internal errors,
452462
// and reference errors from JavaScript evaluation in chrome.
453463
if (result.type.contains('Error')) {
@@ -498,6 +508,7 @@ ${globalLoadStrategy.loadModuleSnippet}("dart_sdk").developer.invokeExtension(
498508

499509
final library = await inspector.getLibrary(targetId);
500510
return await _getEvaluationResult(
511+
isolateId,
501512
() => evaluator.evaluateExpression(
502513
isolateId, library?.uri, expression, scope),
503514
expression);
@@ -521,6 +532,7 @@ ${globalLoadStrategy.loadModuleSnippet}("dart_sdk").developer.invokeExtension(
521532
_checkIsolate('evaluateInFrame', isolateId);
522533

523534
return await _getEvaluationResult(
535+
isolateId,
524536
() => evaluator.evaluateExpressionInFrame(
525537
isolateId, frameIndex, expression, scope),
526538
expression);

‎dwds/lib/src/services/expression_evaluator.dart‎

Lines changed: 26 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ class ExpressionEvaluator {
4141
final Modules _modules;
4242
final ExpressionCompiler _compiler;
4343
final _logger = Logger('ExpressionEvaluator');
44+
bool _closed = false;
4445

4546
/// Strip synthetic library name from compiler error messages.
4647
static final _syntheticNameFilterRegex =
@@ -56,12 +57,14 @@ class ExpressionEvaluator {
5657
ExpressionEvaluator(this._entrypoint, this._inspector, this._debugger,
5758
this._locations, this._modules, this._compiler);
5859

59-
RemoteObject _createError(ErrorKind severity, String message) {
60+
RemoteObject createError(ErrorKind severity, String message) {
6061
return RemoteObject(
6162
<String, String>{'type': '$severity', 'value': message});
6263
}
6364

64-
void close() {}
65+
void close() {
66+
_closed = true;
67+
}
6568

6669
/// Evaluate dart expression inside a given library.
6770
///
@@ -80,19 +83,23 @@ class ExpressionEvaluator {
8083
String expression,
8184
Map<String, String>? scope,
8285
) async {
86+
if (_closed) {
87+
return createError(ErrorKind.internal, 'expression evaluator closed.');
88+
}
89+
8390
scope ??= {};
8491

8592
if (expression.isEmpty) {
86-
return _createError(ErrorKind.invalidInput, expression);
93+
return createError(ErrorKind.invalidInput, expression);
8794
}
8895

8996
if (libraryUri == null) {
90-
return _createError(ErrorKind.invalidInput, 'no library uri');
97+
return createError(ErrorKind.invalidInput, 'no library uri');
9198
}
9299

93100
final module = await _modules.moduleForLibrary(libraryUri);
94101
if (module == null) {
95-
return _createError(ErrorKind.internal, 'no module for $libraryUri');
102+
return createError(ErrorKind.internal, 'no module for $libraryUri');
96103
}
97104

98105
// Wrap the expression in a lambda so we can call it as a function.
@@ -118,7 +125,8 @@ class ExpressionEvaluator {
118125
var result = await _inspector.callFunction(jsCode, scope.values);
119126
result = await _formatEvaluationError(result);
120127

121-
_logger.finest('Evaluated "$expression" to "$result"');
128+
_logger
129+
.finest('Evaluated "$expression" to "$result" for isolate $isolateId');
122130
return result;
123131
}
124132

@@ -139,20 +147,20 @@ class ExpressionEvaluator {
139147
if (scope != null) {
140148
// TODO(annagrin): Implement scope support.
141149
// Issue: https://github.com/dart-lang/webdev/issues/1344
142-
return _createError(
150+
return createError(
143151
ErrorKind.internal,
144152
'Using scope for expression evaluation in frame '
145153
'is not supported.');
146154
}
147155

148156
if (expression.isEmpty) {
149-
return _createError(ErrorKind.invalidInput, expression);
157+
return createError(ErrorKind.invalidInput, expression);
150158
}
151159

152160
// Get JS scope and current JS location.
153161
final jsFrame = _debugger.jsFrameForIndex(frameIndex);
154162
if (jsFrame == null) {
155-
return _createError(
163+
return createError(
156164
ErrorKind.internal,
157165
'Expression evaluation in async frames '
158166
'is not supported. No frame with index $frameIndex.');
@@ -167,12 +175,12 @@ class ExpressionEvaluator {
167175
// Find corresponding dart location and scope.
168176
final url = _debugger.urlForScriptId(jsScriptId);
169177
if (url == null) {
170-
return _createError(
178+
return createError(
171179
ErrorKind.internal, 'Cannot find url for JS script: $jsScriptId');
172180
}
173181
final locationMap = await _locations.locationForJs(url, jsLine, jsColumn);
174182
if (locationMap == null) {
175-
return _createError(
183+
return createError(
176184
ErrorKind.internal,
177185
'Cannot find Dart location for JS location: '
178186
'url: $url, '
@@ -185,13 +193,13 @@ class ExpressionEvaluator {
185193
final dartSourcePath = dartLocation.uri.serverPath;
186194
final libraryUri = await _modules.libraryForSource(dartSourcePath);
187195
if (libraryUri == null) {
188-
return _createError(
196+
return createError(
189197
ErrorKind.internal, 'no libraryUri for $dartSourcePath');
190198
}
191199

192200
final module = await _modules.moduleForLibrary(libraryUri.toString());
193201
if (module == null) {
194-
return _createError(
202+
return createError(
195203
ErrorKind.internal, 'no module for $libraryUri ($dartSourcePath)');
196204
}
197205

@@ -246,21 +254,21 @@ class ExpressionEvaluator {
246254
}
247255
if (error.contains('InternalError: ')) {
248256
error = error.replaceAll('InternalError: ', '');
249-
return _createError(ErrorKind.internal, error);
257+
return createError(ErrorKind.internal, error);
250258
}
251259
error = error.replaceAll(_syntheticNameFilterRegex, '');
252-
return _createError(ErrorKind.compilation, error);
260+
return createError(ErrorKind.compilation, error);
253261
}
254262

255263
Future<RemoteObject> _formatEvaluationError(RemoteObject result) async {
256264
if (result.type == 'string') {
257265
var error = '${result.value}';
258266
if (error.startsWith('ReferenceError: ')) {
259267
error = error.replaceFirst('ReferenceError: ', '');
260-
return _createError(ErrorKind.reference, error);
268+
return createError(ErrorKind.reference, error);
261269
} else if (error.startsWith('TypeError: ')) {
262270
error = error.replaceFirst('TypeError: ', '');
263-
return _createError(ErrorKind.type, error);
271+
return createError(ErrorKind.type, error);
264272
} else if (error.startsWith('NetworkError: ')) {
265273
var modulePath = _loadModuleErrorRegex.firstMatch(error)?.group(1);
266274
final module = modulePath != null
@@ -271,7 +279,7 @@ class ExpressionEvaluator {
271279
error = 'Module is not loaded : $module (path: $modulePath). '
272280
'Accessing libraries that have not yet been used in the '
273281
'application is not supported during expression evaluation.';
274-
return _createError(ErrorKind.loadModule, error);
282+
return createError(ErrorKind.loadModule, error);
275283
}
276284
}
277285
return result;

‎dwds/test/expression_evaluator_test.dart‎

Lines changed: 137 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,137 @@
1+
// Copyright (c) 2020, the Dart project authors. Please see the AUTHORS file
2+
// for details. All rights reserved. Use of this source code is governed by a
3+
// BSD-style license that can be found in the LICENSE file.
4+
5+
@TestOn('vm')
6+
@Timeout(Duration(minutes: 2))
7+
import 'dart:async';
8+
9+
import 'package:dwds/src/debugging/debugger.dart';
10+
import 'package:dwds/src/debugging/location.dart';
11+
import 'package:dwds/src/debugging/skip_list.dart';
12+
import 'package:dwds/src/loaders/strategy.dart';
13+
import 'package:dwds/src/services/batched_expression_evaluator.dart';
14+
import 'package:dwds/src/services/expression_evaluator.dart';
15+
16+
import 'package:test/test.dart';
17+
import 'package:webkit_inspection_protocol/webkit_inspection_protocol.dart';
18+
19+
import 'fixtures/fakes.dart';
20+
21+
late ExpressionEvaluator? _evaluator;
22+
late ExpressionEvaluator? _batchedEvaluator;
23+
24+
void main() async {
25+
group('expression compiler service with fake asset server', () {
26+
Future<void> stop() async {
27+
_evaluator?.close();
28+
_evaluator = null;
29+
_batchedEvaluator?.close();
30+
_batchedEvaluator = null;
31+
}
32+
33+
setUp(() async {
34+
globalLoadStrategy = FakeStrategy();
35+
36+
final assetReader = FakeAssetReader(sourceMap: '');
37+
final modules = FakeModules();
38+
39+
final webkitDebugger = FakeWebkitDebugger(scripts: {});
40+
final pausedController = StreamController<DebuggerPausedEvent>();
41+
webkitDebugger.onPaused = pausedController.stream;
42+
43+
final root = 'fakeRoot';
44+
final entry = 'fake_entrypoint';
45+
final locations = Locations(assetReader, modules, root);
46+
locations.initialize(entry);
47+
48+
final skipLists = SkipLists();
49+
final debugger = await Debugger.create(
50+
webkitDebugger,
51+
(_, __) {},
52+
locations,
53+
skipLists,
54+
root,
55+
);
56+
final inspector = FakeInspector(fakeIsolate: simpleIsolate);
57+
debugger.updateInspector(inspector);
58+
59+
_evaluator = ExpressionEvaluator(
60+
entry,
61+
inspector,
62+
debugger,
63+
locations,
64+
modules,
65+
FakeExpressionCompiler(),
66+
);
67+
_batchedEvaluator = BatchedExpressionEvaluator(
68+
entry,
69+
inspector,
70+
debugger,
71+
locations,
72+
modules,
73+
FakeExpressionCompiler(),
74+
);
75+
});
76+
77+
tearDown(() async {
78+
await stop();
79+
});
80+
81+
group('evaluator', () {
82+
late ExpressionEvaluator evaluator;
83+
84+
setUp(() async {
85+
evaluator = _evaluator!;
86+
});
87+
88+
test('can evaluate expression', () async {
89+
final result =
90+
await evaluator.evaluateExpression('1', 'main.dart', 'true', {});
91+
expect(
92+
result,
93+
const TypeMatcher<RemoteObject>()
94+
.having((o) => o.value, 'value', 'true'));
95+
});
96+
97+
test('returns error if closed', () async {
98+
evaluator.close();
99+
final result =
100+
await evaluator.evaluateExpression('1', 'main.dart', 'true', {});
101+
expect(
102+
result,
103+
const TypeMatcher<RemoteObject>()
104+
.having((o) => o.type, 'type', 'InternalError')
105+
.having((o) => o.value, 'value', contains('evaluator closed')));
106+
});
107+
});
108+
109+
group('batched evaluator', () {
110+
late ExpressionEvaluator evaluator;
111+
112+
setUp(() async {
113+
evaluator = _batchedEvaluator!;
114+
});
115+
116+
test('can evaluate expression', () async {
117+
final result =
118+
await evaluator.evaluateExpression('1', 'main.dart', 'true', {});
119+
expect(
120+
result,
121+
const TypeMatcher<RemoteObject>()
122+
.having((o) => o.value, 'value', 'true'));
123+
});
124+
125+
test('returns error if closed', () async {
126+
evaluator.close();
127+
final result =
128+
await evaluator.evaluateExpression('1', 'main.dart', 'true', {});
129+
expect(
130+
result,
131+
const TypeMatcher<RemoteObject>()
132+
.having((o) => o.type, 'type', 'InternalError')
133+
.having((o) => o.value, 'value', contains('evaluator closed')));
134+
});
135+
});
136+
});
137+
}

0 commit comments

Comments
 (0)