[coverage] Finish collection as soon as main isolate exits (#2069)
diff --git a/pkgs/coverage/CHANGELOG.md b/pkgs/coverage/CHANGELOG.md index 86e248e..7c880fb 100644 --- a/pkgs/coverage/CHANGELOG.md +++ b/pkgs/coverage/CHANGELOG.md
@@ -1,6 +1,9 @@ ## 1.12.0-wip - Introduced support for specifying coverage flags through a YAML file. +- Fix a [bug](https://github.com/dart-lang/tools/issues/2049) where coverage + collection could hang forever if the package under test used background + isolates that were never shut down. ## 1.11.1
diff --git a/pkgs/coverage/lib/src/isolate_paused_listener.dart b/pkgs/coverage/lib/src/isolate_paused_listener.dart index 6f538a4..28894f7 100644 --- a/pkgs/coverage/lib/src/isolate_paused_listener.dart +++ b/pkgs/coverage/lib/src/isolate_paused_listener.dart
@@ -28,30 +28,41 @@ final _isolateGroups = <String, IsolateGroupState>{}; - int _numNonMainIsolates = 0; - final _allNonMainIsolatesExited = Completer<void>(); - bool _finished = false; + int _numIsolates = 0; + bool _finishedListening = false; IsolateRef? _mainIsolate; - final _mainIsolatePaused = Completer<bool>(); + bool _hasMainIsolate = false; + + // Completes when either: + // - there is a main isolate, and it is paused or exited + // - there is no main isolate, and all isolates have exited + final _mainIsolatePausedOrAllIsolatesExited = Completer<void>(); /// Starts listening and returns a future that completes when all isolates /// have exited. Future<void> waitUntilAllExited() async { await listenToIsolateLifecycleEvents(_service, _onStart, _onPause, _onExit); - await _allNonMainIsolatesExited.future; + await _mainIsolatePausedOrAllIsolatesExited.future; + _finishedListening = true; - // Resume the main isolate. - try { - if (_mainIsolate != null) { - if (await _mainIsolatePaused.future) { - await _runCallbackAndResume( - _mainIsolate!, !_getGroup(_mainIsolate!).collected); + // Collect all remaining uncollected groups. + final collectionTasks = <Future<void>>[]; + for (final group in _isolateGroups.values) { + if (!group.collected) { + group.collected = true; + final iso = group.paused.firstOrNull ?? group.running.firstOrNull; + if (iso != null) { + collectionTasks.add(_onIsolatePaused(iso, true)); } } - } finally { - _finished = true; + } + await Future.wait(collectionTasks); + + // Resume the main isolate. + if (_mainIsolate != null) { + await _service.resume(_mainIsolate!.id!); } } @@ -59,70 +70,51 @@ _isolateGroups[isolateRef.isolateGroupId!] ??= IsolateGroupState(); void _onStart(IsolateRef isolateRef) { - if (_finished) return; + if (_finishedListening) return; final group = _getGroup(isolateRef); - group.start(isolateRef.id!); - if (_mainIsolate == null && _isMainIsolate(isolateRef)) { + group.start(isolateRef); + ++_numIsolates; + if (!_hasMainIsolate && _isMainIsolate(isolateRef)) { _mainIsolate = isolateRef; - } else { - ++_numNonMainIsolates; + _hasMainIsolate = true; } } Future<void> _onPause(IsolateRef isolateRef) async { - if (_finished) return; + if (_finishedListening) return; final group = _getGroup(isolateRef); - group.pause(isolateRef.id!); + group.pause(isolateRef); if (isolateRef.id! == _mainIsolate?.id) { - _mainIsolatePaused.complete(true); - - // If the main isolate is the only isolate, then _allNonMainIsolatesExited - // will never be completed. So check that case here. - _checkCompleted(); + _mainIsolatePausedOrAllIsolatesExited.safeComplete(); } else { - await _runCallbackAndResume(isolateRef, group.noRunningIsolates); - } - } - - Future<void> _runCallbackAndResume( - IsolateRef isolateRef, bool isLastIsolateInGroup) async { - if (isLastIsolateInGroup) { - _getGroup(isolateRef).collected = true; - } - try { - await _onIsolatePaused(isolateRef, isLastIsolateInGroup); - } finally { - await _service.resume(isolateRef.id!); + final isLastIsolateInGroup = group.noRunningIsolates; + if (isLastIsolateInGroup) { + _getGroup(isolateRef).collected = true; + } + try { + await _onIsolatePaused(isolateRef, isLastIsolateInGroup); + } finally { + group.exit(isolateRef); + await _service.resume(isolateRef.id!); + } } } void _onExit(IsolateRef isolateRef) { - if (_finished) return; + if (_finishedListening) return; final group = _getGroup(isolateRef); - group.exit(isolateRef.id!); + group.exit(isolateRef); + --_numIsolates; if (group.noLiveIsolates && !group.collected) { _log('ERROR: An isolate exited without pausing, causing ' 'coverage data to be lost for group ${isolateRef.isolateGroupId!}.'); } if (isolateRef.id! == _mainIsolate?.id) { - if (!_mainIsolatePaused.isCompleted) { - // Main isolate exited without pausing. - _mainIsolatePaused.complete(false); - } - } else { - --_numNonMainIsolates; - _checkCompleted(); - } - } - - bool get _mainRunning => - _mainIsolate != null && !_mainIsolatePaused.isCompleted; - - void _checkCompleted() { - if (_numNonMainIsolates == 0 && - !_mainRunning && - !_allNonMainIsolatesExited.isCompleted) { - _allNonMainIsolatesExited.complete(); + // Main isolate exited without pausing. + _mainIsolate = null; + _mainIsolatePausedOrAllIsolatesExited.safeComplete(); + } else if (!_hasMainIsolate && _numIsolates == 0) { + _mainIsolatePausedOrAllIsolatesExited.safeComplete(); } } @@ -139,6 +131,12 @@ } } +extension on Completer { + void safeComplete() { + if (!isCompleted) complete(); + } +} + /// Listens to isolate start and pause events, and backfills events for isolates /// that existed before listening started. /// @@ -220,30 +218,30 @@ class IsolateGroupState { // IDs of the isolates running in this group. @visibleForTesting - final running = <String>{}; + final running = <IsolateRef>{}; // IDs of the isolates paused just before exiting in this group. @visibleForTesting - final paused = <String>{}; + final paused = <IsolateRef>{}; bool collected = false; bool get noRunningIsolates => running.isEmpty; bool get noLiveIsolates => running.isEmpty && paused.isEmpty; - void start(String id) { - paused.remove(id); - running.add(id); + void start(IsolateRef iso) { + paused.remove(iso); + running.add(iso); } - void pause(String id) { - running.remove(id); - paused.add(id); + void pause(IsolateRef iso) { + running.remove(iso); + paused.add(iso); } - void exit(String id) { - running.remove(id); - paused.remove(id); + void exit(IsolateRef iso) { + running.remove(iso); + paused.remove(iso); } @override
diff --git a/pkgs/coverage/test/collect_coverage_test.dart b/pkgs/coverage/test/collect_coverage_test.dart index 7262757..476fee1 100644 --- a/pkgs/coverage/test/collect_coverage_test.dart +++ b/pkgs/coverage/test/collect_coverage_test.dart
@@ -133,24 +133,25 @@ 39: 1, 41: 1, 42: 4, - 43: 1, - 44: 3, - 45: 1, - 48: 1, + 43: 2, + 44: 1, + 45: 3, + 46: 1, 49: 1, - 51: 1, - 54: 1, + 50: 1, + 52: 1, 55: 1, 56: 1, - 59: 1, + 57: 1, 60: 1, - 62: 1, + 61: 1, 63: 1, 64: 1, - 66: 1, + 65: 1, 67: 1, 68: 1, - 71: 3, + 69: 1, + 72: 3, }; expect(isolateFile?.lineHits, expectedHits); expect(isolateFile?.funcHits, {11: 1, 19: 1, 23: 1, 28: 1, 38: 1}); @@ -174,7 +175,7 @@ 32: 0, 38: 1, 42: 1, - 71: 1, + 72: 1, }, ); });
diff --git a/pkgs/coverage/test/isolate_paused_listener_test.dart b/pkgs/coverage/test/isolate_paused_listener_test.dart index b6016ce..dc77408 100644 --- a/pkgs/coverage/test/isolate_paused_listener_test.dart +++ b/pkgs/coverage/test/isolate_paused_listener_test.dart
@@ -129,73 +129,78 @@ expect(group.noRunningIsolates, isTrue); expect(group.noLiveIsolates, isTrue); - group.start('a'); - expect(group.running, unorderedEquals(['a'])); + group.start(IsolateRef(id: 'a')); + expect(group.running.map((iso) => iso.id).toList(), unorderedEquals(['a'])); expect(group.paused, isEmpty); expect(group.noRunningIsolates, isFalse); expect(group.noLiveIsolates, isFalse); - group.start('a'); - expect(group.running, unorderedEquals(['a'])); + group.start(IsolateRef(id: 'a')); + expect(group.running.map((iso) => iso.id).toList(), unorderedEquals(['a'])); expect(group.paused, isEmpty); expect(group.noRunningIsolates, isFalse); expect(group.noLiveIsolates, isFalse); - group.start('b'); - expect(group.running, unorderedEquals(['a', 'b'])); + group.start(IsolateRef(id: 'b')); + expect(group.running.map((iso) => iso.id).toList(), + unorderedEquals(['a', 'b'])); expect(group.paused, isEmpty); expect(group.noRunningIsolates, isFalse); expect(group.noLiveIsolates, isFalse); - group.pause('a'); - expect(group.running, unorderedEquals(['b'])); - expect(group.paused, unorderedEquals(['a'])); + group.pause(IsolateRef(id: 'a')); + expect(group.running.map((iso) => iso.id).toList(), unorderedEquals(['b'])); + expect(group.paused.map((iso) => iso.id).toList(), unorderedEquals(['a'])); expect(group.noRunningIsolates, isFalse); expect(group.noLiveIsolates, isFalse); - group.pause('a'); - expect(group.running, unorderedEquals(['b'])); - expect(group.paused, unorderedEquals(['a'])); + group.pause(IsolateRef(id: 'a')); + expect(group.running.map((iso) => iso.id).toList(), unorderedEquals(['b'])); + expect(group.paused.map((iso) => iso.id).toList(), unorderedEquals(['a'])); expect(group.noRunningIsolates, isFalse); expect(group.noLiveIsolates, isFalse); - group.pause('c'); - expect(group.running, unorderedEquals(['b'])); - expect(group.paused, unorderedEquals(['a', 'c'])); + group.pause(IsolateRef(id: 'c')); + expect(group.running.map((iso) => iso.id).toList(), unorderedEquals(['b'])); + expect(group.paused.map((iso) => iso.id).toList(), + unorderedEquals(['a', 'c'])); expect(group.noRunningIsolates, isFalse); expect(group.noLiveIsolates, isFalse); - group.start('c'); - expect(group.running, unorderedEquals(['b', 'c'])); - expect(group.paused, unorderedEquals(['a'])); + group.start(IsolateRef(id: 'c')); + expect(group.running.map((iso) => iso.id).toList(), + unorderedEquals(['b', 'c'])); + expect(group.paused.map((iso) => iso.id).toList(), unorderedEquals(['a'])); expect(group.noRunningIsolates, isFalse); expect(group.noLiveIsolates, isFalse); - group.pause('c'); - expect(group.running, unorderedEquals(['b'])); - expect(group.paused, unorderedEquals(['a', 'c'])); + group.pause(IsolateRef(id: 'c')); + expect(group.running.map((iso) => iso.id).toList(), unorderedEquals(['b'])); + expect(group.paused.map((iso) => iso.id).toList(), + unorderedEquals(['a', 'c'])); expect(group.noRunningIsolates, isFalse); expect(group.noLiveIsolates, isFalse); - group.exit('a'); - expect(group.running, unorderedEquals(['b'])); - expect(group.paused, unorderedEquals(['c'])); + group.exit(IsolateRef(id: 'a')); + expect(group.running.map((iso) => iso.id).toList(), unorderedEquals(['b'])); + expect(group.paused.map((iso) => iso.id).toList(), unorderedEquals(['c'])); expect(group.noRunningIsolates, isFalse); expect(group.noLiveIsolates, isFalse); - group.pause('b'); + group.pause(IsolateRef(id: 'b')); expect(group.running, isEmpty); - expect(group.paused, unorderedEquals(['b', 'c'])); + expect(group.paused.map((iso) => iso.id).toList(), + unorderedEquals(['b', 'c'])); expect(group.noRunningIsolates, isTrue); expect(group.noLiveIsolates, isFalse); - group.exit('b'); + group.exit(IsolateRef(id: 'b')); expect(group.running, isEmpty); - expect(group.paused, unorderedEquals(['c'])); + expect(group.paused.map((iso) => iso.id).toList(), unorderedEquals(['c'])); expect(group.noRunningIsolates, isTrue); expect(group.noLiveIsolates, isFalse); - group.exit('c'); + group.exit(IsolateRef(id: 'c')); expect(group.running, isEmpty); expect(group.paused, isEmpty); expect(group.noRunningIsolates, isTrue); @@ -528,7 +533,7 @@ service, (iso, isLastIsolateInGroup) async { expect(stopped, isFalse); - received.add('Pause ${iso.id}. Last in group ${iso.isolateGroupId}? ' + received.add('Pause ${iso.id}. Collect group ${iso.isolateGroupId}? ' '${isLastIsolateInGroup ? 'Yes' : 'No'}'); }, (message) => received.add(message), @@ -572,23 +577,23 @@ exitEvent('Z', '9'); expect(received, [ - 'Pause A. Last in group 1? No', + 'Pause A. Collect group 1? No', 'Resume A', - 'Pause B. Last in group 1? No', + 'Pause B. Collect group 1? No', 'Resume B', - 'Pause C. Last in group 1? Yes', + 'Pause C. Collect group 1? Yes', 'Resume C', - 'Pause F. Last in group 2? No', + 'Pause F. Collect group 2? No', 'Resume F', - 'Pause E. Last in group 2? No', + 'Pause E. Collect group 2? No', 'Resume E', - 'Pause I. Last in group 3? No', + 'Pause I. Collect group 3? No', 'Resume I', - 'Pause H. Last in group 3? No', + 'Pause H. Collect group 3? No', 'Resume H', - 'Pause D. Last in group 2? Yes', + 'Pause D. Collect group 2? Yes', 'Resume D', - 'Pause G. Last in group 3? Yes', + 'Pause G. Collect group 3? Yes', 'Resume G', ]); }); @@ -614,16 +619,16 @@ // B was paused correctly and was the last to exit isolate 1, so isolate 1 // was collected ok. expect(received, [ - 'Pause B. Last in group 1? Yes', + 'Pause B. Collect group 1? Yes', 'Resume B', - 'Pause D. Last in group 2? No', + 'Pause D. Collect group 2? No', 'Resume D', 'ERROR: An isolate exited without pausing, causing coverage data to ' 'be lost for group 2.', ]); }); - test('main isolate resumed last', () async { + test('subsequent main isolates are ignored', () async { startEvent('A', '1', 'main'); startEvent('B', '1', 'main'); // Second isolate named main, ignored. pauseEvent('B', '1', 'main'); @@ -639,17 +644,52 @@ await endTest(); expect(received, [ - 'Pause B. Last in group 1? No', + 'Pause B. Collect group 1? No', 'Resume B', - 'Pause C. Last in group 2? No', - 'Resume C', - 'Pause D. Last in group 2? Yes', - 'Resume D', - 'Pause A. Last in group 1? Yes', + 'Pause A. Collect group 1? Yes', + 'Pause C. Collect group 2? Yes', 'Resume A', ]); }); + test('all groups collected as soon as main isolate paused', () async { + startEvent('B', '1'); + startEvent('E', '2'); + startEvent('D', '2'); + startEvent('F', '2'); + pauseEvent('F', '2'); + startEvent('C', '2'); + startEvent('A', '1'); + pauseEvent('B', '1'); + pauseEvent('E', '2'); + startEvent('G', '3'); + pauseEvent('D', '2'); + startEvent('H', '3'); + exitEvent('F', '2'); + startEvent('I', '3'); + exitEvent('I', '3'); + + startEvent('M', '1', 'main'); + pauseEvent('M', '1', 'main'); + + await endTest(); + + expect(received, [ + 'Pause F. Collect group 2? No', + 'Resume F', + 'Pause B. Collect group 1? No', + 'Resume B', + 'Pause E. Collect group 2? No', + 'Resume E', + 'Pause D. Collect group 2? No', + 'Resume D', + 'Pause M. Collect group 1? Yes', + 'Pause C. Collect group 2? Yes', + 'Pause G. Collect group 3? Yes', + 'Resume M', + ]); + }); + test('main isolate exits without pausing', () async { startEvent('A', '1', 'main'); startEvent('B', '1'); @@ -660,7 +700,7 @@ await endTest(); expect(received, [ - 'Pause B. Last in group 1? No', + 'Pause B. Collect group 1? No', 'Resume B', 'ERROR: An isolate exited without pausing, causing coverage data to ' 'be lost for group 1.', @@ -674,7 +714,7 @@ await endTest(); expect(received, [ - 'Pause A. Last in group 1? Yes', + 'Pause A. Collect group 1? Yes', 'Resume A', ]); }); @@ -693,9 +733,9 @@ await endTest(); expect(received, [ - 'Pause B. Last in group 1? No', + 'Pause B. Collect group 1? No', 'Resume B', - 'Pause A. Last in group 1? Yes', + 'Pause A. Collect group 1? Yes', 'Resume A', ]); }); @@ -722,11 +762,11 @@ await endTest(); expect(received, [ - 'Pause B. Last in group 1? No', + 'Pause B. Collect group 1? No', 'Resume B', - 'Pause C. Last in group 1? No', + 'Pause C. Collect group 1? No', 'Resume C', - 'Pause A. Last in group 1? Yes', + 'Pause A. Collect group 1? Yes', 'Resume A', ]); }); @@ -753,11 +793,9 @@ await endTest(); expect(received, [ - 'Pause B. Last in group 1? No', + 'Pause B. Collect group 1? No', 'Resume B', - 'Pause C. Last in group 1? Yes', - 'Resume C', - 'Pause A. Last in group 1? No', + 'Pause A. Collect group 1? Yes', 'Resume A', ]); }); @@ -795,19 +833,19 @@ await endTest(); expect(received, [ - 'Pause Z. Last in group 9? Yes', + 'Pause Z. Collect group 9? Yes', 'Resume Z', - 'Pause A. Last in group 1? No', + 'Pause A. Collect group 1? No', 'Resume A', - 'Pause B. Last in group 1? Yes', + 'Pause B. Collect group 1? Yes', 'Resume B', - 'Pause E. Last in group 2? No', + 'Pause E. Collect group 2? No', 'Resume E', - 'Pause D. Last in group 2? Yes', + 'Pause D. Collect group 2? Yes', 'Resume D', - 'Pause F. Last in group 2? Yes', + 'Pause F. Collect group 2? Yes', 'Resume F', - 'Pause C. Last in group 1? Yes', + 'Pause C. Collect group 1? Yes', 'Resume C', ]); });
diff --git a/pkgs/coverage/test/run_and_collect_test.dart b/pkgs/coverage/test/run_and_collect_test.dart index e371f90..a538a88 100644 --- a/pkgs/coverage/test/run_and_collect_test.dart +++ b/pkgs/coverage/test/run_and_collect_test.dart
@@ -84,10 +84,10 @@ expect(ignoredLinesInFilesCache[packageUtilKey], isEmpty); expect(ignoredLinesInFilesCache[testAppKey], null /* means whole file */); expect(ignoredLinesInFilesCache[testAppIsolateKey], [ - [51, 51], - [53, 57], - [62, 65], - [66, 72] + [52, 52], + [54, 58], + [63, 66], + [67, 73] ]); } @@ -112,13 +112,14 @@ 39: 1, 41: 1, 42: 4, - 43: 1, - 44: 3, - 45: 1, - 48: 1, + 43: 2, + 44: 1, + 45: 3, + 46: 1, 49: 1, - 59: 1, - 60: 1 + 50: 1, + 60: 1, + 61: 1 }; expect(actualLineHits, expectedLineHits);
diff --git a/pkgs/coverage/test/test_files/test_app_isolate.dart b/pkgs/coverage/test/test_files/test_app_isolate.dart index 8bbc6be..22ccd75 100644 --- a/pkgs/coverage/test/test_files/test_app_isolate.dart +++ b/pkgs/coverage/test/test_files/test_app_isolate.dart
@@ -40,6 +40,7 @@ fooSync(answer); unawaited(fooAsync(answer).then((_) { + RawReceivePort().keepIsolateAlive = true; final port = threeThings.first as SendPort; final sum = (threeThings[1] as int) + (threeThings[2] as int); port.send(sum);